tmsriram added a comment. In D92633#2434511 <https://reviews.llvm.org/D92633#2434511>, @MaskRay wrote:
> 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. Correct me if I am wrong, but I do see that this behavior is touched. Line 10 in -fdirect-access-external-data.c : // RUN: %clang -### -c -target aarch64 %s -fpic -fdirect-access-external-data 2>&1 | FileCheck %s --check-prefix=DIRECT With -fpic, the variable access will go directly and not via GOT. > 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
