Reland "[flang] Avoid undefined behaviour when parsing format expressions (#147539)" #148169
Conversation
…ions (llvm#147539)" This reverts commit e8e5d07. This previously failed because the flang-rt build could not find the llvm header file. It passed on some machines but only because they had globally installed copies of older llvm. To fix this, I've copied the required routines from llvm into flang. With the following justification: * Flang can, and does, use llvm headers. * Some Flang headers are also used in Flang-rt. * Flang-rt cannot use llvm headers. * Therefore any Flang header using in Flang-rt cannot use llvm headers either. To support that conclusion, https://flang.llvm.org/docs/IORuntimeInternals.html states: "The Fortran I/O runtime support library is written in C++17, and uses some C++17 standard library facilities, but it is intended to not have any link-time dependences on the C++ runtime support library or any LLVM libraries." This talks about libraries but I assume it applies to llvm in general. Nothing in flang/include/flang/Common, or flang/include/flang/Common includes any llvm header, and I see some very similar headers there that duplicate llvm functionality. Like float128.h. I can only assume this means these files must remain free of dependencies on LLVM. I have copied the two routines literally and put them in the flang::common namespace, for lack of a better place for them. So they don't clash with something. I have specialised the function to the 1 type flang needs, as it might save a bit of compile time.
|
The template sepcialisation is in its own commit in case it's easier to check that way. |
|
The intention of the rule is to allow people to run fortran programs on systems that do not have C++ standard libraries installed. A header-only dependency on these LLVM functions should be okay because they don't call anything else. Similarly, header-only dependencies on libc++ are okay (like the use of std::numeric_limits here) because that won't create a runtime dependency between flang-rt and the c++ library. But now that you mention it, I agree this is a less brittle approach so that we don't accidentally end up with runtime dependencies on libc++ because of a change in an llvm header. However, currently I am getting "multiple definitions of Fortran::common::AddOverflow" when linking flang-rt with your change. I am linking using /usr/bin/ld, building with gcc, and |
DavidSpickett
force-pushed
the
flang-reland
branch
from
e9e5e8b to
4616aed
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
@tblah try it again, I think the static I added should fix it. It's that or make them members of the using class, which isn't an awful idea either but harder to find later. Did not fail in my local build but I see why it could, pure chance that it doesn't I expect. |
|
Thanks, I'll land this Monday when I am here to catch any fallout.
Is this something the build checks? Idk if you can just look at what flang-rt links to, or whether you have to link a fortran program and look at that. If it's not documented how to check that, it would be nice to have written down. |
|
I'm not sure how to check what a static library links to. If it is dynamic you can use I dimly remember there being a bot somewhere that kept an eye on these dependencies for flang-rt but whenever I heard about it was a long time ago. Edit: maybe it would be more robust to objdump the static library and scan for symbols mangled to look like they come from libc++ |
|
This breaks building the flang runtime for the device with the following error: Also the flang runtime codebase uses brace initialization. |
This reverts commit e8e5d07.
This previously failed because the flang-rt build could not find the
llvm header file. It passed on some machines but only because they
had globally installed copies of older llvm.
To fix this, I've copied the required routines from llvm into flang.
With the following justification:
To support that conclusion, https://flang.llvm.org/docs/IORuntimeInternals.html
states:
"The Fortran I/O runtime support library is written in C++17, and uses some C++17 standard library facilities, but it is intended to not have any link-time dependences on the C++ runtime support library or any LLVM libraries."
This talks about libraries but I assume it applies to llvm in general.
Nothing in flang/include/flang/Common, or flang/include/flang/Common
includes any llvm header, and I see some very similar headers there
that duplicate llvm functionality. Like float128.h.
I can only assume this means these files must remain free of dependencies
on LLVM.
I have copied the two routines literally and put them in the flang::common
namespace, for lack of a better place for them. So they don't clash with something.
I have specialised the function to the 1 type flang needs, as it might
save a bit of compile time.