[PATCH] D92633: Add -f[no-]direct-access-external-data to deprecate -mpie-copy-relocations

Fri Dec 4 13:21:25 PST 2020

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