MaskRay added a comment. In D92633#2434337 <https://reviews.llvm.org/D92633#2434337>, @tmsriram wrote:
> In D92633#2434267 <https://reviews.llvm.org/D92633#2434267>, @MaskRay wrote: > >> In D92633#2434231 <https://reviews.llvm.org/D92633#2434231>, @tmsriram wrote: >> >>> Sorry just one more thing which is a bit concerning: >>> >>> When I do : >>> >>> $ clang -fPIC -frxternal-data-access foo.c >>> >>> You will omit the GOT but this object can go into a shared object and break >>> the build as this does not apply to shared objects? Should we allow this >>> at all for -fPIC? I dont see a need for this but if you want to, maybe >>> warn? The user should know that this cannot go into a shared object right? >>> I am also ok with commenting that this option can do bad things for -fPIC >>> and the user should know what they are doing. >> >> `clang -fPIC -fdirect-access-eternal-data -c a.c; ld -no-pie a.o; ld -pie >> a.o` (producing an executable with either `-no-pie` or `-pie`) is fine. > > Yes, agreed. Putting this object into an executable (pie/no-pie) no matter > how you compile it is always fine as long as the backend supports copy > relocations. No issues there. > >> For `-shared`, because an ELF linker assumes interposition for non-local >> STV_DEFAULT symbols by default, linking such an object files requires some >> mechanism to make it non-preemptible. > > Right, that was my point. Without -fPIC, we can be guaranteed that it won't > go into a shared object and this case wouldn't arise. > >> The simplest is `clang -fPIC -fdirect-access-external-data -c a.c; ld >> -shared -Bsymbolic a.o` >> Other mechanisms include: `--dynamic-list` (don't list the symbol) and >> `--version-script` (localize the symbol). >> This is going to be tricky and I don't know how to fit the information into >> the few-line description of the option. > > How about making this option applicable for -fpie/fPIE and the default case > and *not allowing* it for -fPIC/-fpic? That seems the safest approach. > >> I just checked how to make -fdirect-access-eternal-data work for -fno-pic >> (both TargetMachine::shouldAssumeDSOLocal and >> CodeGenModule.cpp:shouldAssumeDSOLocal should not assume false for >> Reloc::Static), unfortunately there are 200 tests needing updates. (This >> reminds me that LLVM doesn't get the default for dso_local right, so many >> tests have `dso_local` in ELF/COFF but no `dso_local` in Mach-O. >> Unfortunately it is extremely difficult to fix it today (D76561 >> <https://reviews.llvm.org/D76561>)) > > Ok, I lost you here a bit. This should be fine for -fno-pic was my > understanding. > > Let me try to summarize the motivations of this patch: > > 1. Originally, the default build (fno-pie/fno-pic), always used direct access > for external globals resulting in copy relocations if it is truly external at > link time. You want to change that to be able to not have copy relocations > with -fno-direct-access-external-data, and you claim this would be useful for > POWER, great! > 2. Originally, with -fPIE and -fpie, -mpie-copy-relocations allowed direct > access to externals. This was always safe because the object was guaranteed > to go into an executable. The new option preserves this behavior so there is > **no concern**. Yes for 1 and 2. This patch only makes the options work for -fpie (as the original -mpie-copy-relocations does). 1 will be a nice cleanup (to drop 2 lines from TargetMachine::shouldAssumeDSOLocal) but it may require some test updates and it looks like `CodeGen/MachineCSE.cpp` exposes a crashing bug that it cannot handle non-dso_local `external constant` in -relocation-model=static mode correctly... > 3. Originally, there was no way to generate direct accesses to external > global variables with -fPIC/-fpic. That behavior will change if you support > -fdirect-access-external-data with -fPIC. **That is my concern that this adds > to the complexity and the user should know what they are doing.** > > Given 3), I am wondering if you really need this patch. I will leave it to > you but please do consider the fact that opening up this option to -fPIC > might be a problem. I agree. The behavior is not touched in this patch. I think the existing -mpie-copy-relocations users should be aware that the option (-fdirect-access-external-data) should generally only be used with -fno-pic and -fpie. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92633/new/ https://reviews.llvm.org/D92633 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
