tmsriram added a comment. 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**. 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. 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
