- User Since
- Oct 24 2016, 8:15 AM (168 w, 4 d)
Thu, Jan 9
Thanks for doing this. I think this approach in much safer at the cost of some friction on the systems where the option is needed. My one suggestion would be to separate the tail call case b foo from the normal call case. It will cause a runtime failure if the callers caller resides outside the dso and would be much more likely then a failure like glibc encountered. I believe gcc/gfortran got that case correct so it shouldn't impact usability on the older OS releases, but I'm not 100% sure of that.
I think the change is correct, and desirable for the reasons you mentioned previously: we get consistent semantics whether function sections are used or not, and consistent behavior whether we use the integrated assembler or not. There is a chance this patch affects some users negatively despite being correct. Ideally we would commit to at least supporting part of the semantic-interposition option (so the user can opt in to disabling IPO when the callee is runtime replaceable) in the same release we land this fix. If I had the bandwidth to do this I would offer but I don't :(. Because of that user impact I don't feel confident enough to approve the patch myself, but I have no strong objection against it landing.
Wed, Jan 8
LGTM: I really didn't like setting PIC as the default just to work around some codegen bugs. I believe there are PowerPC codegen tests that will fail with this change though (or perhaps there is a PowerPC leak sanitizer build bot which begins to fail? @stefanp probably knows what exposed the BE issue previously).
Mon, Jan 6
Fri, Jan 3
I agree that the runtime symbol preemption makes the failure unlikely. I disagree its not a bug though: it doesn't matter that FORTRAN and C++ have the 'one-definition-rule' because the generated code isn't ABI compliant. A compiler producing a valid C++/Fortran program that isn't ABI compliant is definitely a bug. The case I am much more concerned about is code like this:
Thu, Jan 2
I can definitely see it from the opposite side: We want LLD to be usable on these systems that shipped with this bug in libstdc++ and gcc/gfortran. And adding a link option can be hard to do in some build setups so its not as simple as I make it sound. What worries me though is that one of the compilers could subtly break and with the linker[s] being sloppy with the error then we don't find out until long after the toolchain has shipped.
Relaxing the check will allow those old miss-compiled objects to link, but we are loosing some utility here as well: we are turning a valid link time failure into a potential runtime failure. Even worse a runtime failure that depends on the environment at load time so the failure is not consistent. Could we guard the error relaxation with a specific option so that 1) It must be opted into specifically and 2) We can eventually phase the option out once the OS that used gcc 5.* and 6.* go out of service?
I think we should be safe removing the Darwin code in this patch, but Chris will know better.
Fri, Dec 20
Uploaded wrong diff last time.
Rebased and added '-verify-machineinstrs` to the run commands.
Dec 18 2019
In terms of making a dso_local ifunc work this behavior seems logical to me: I'm skeptical though because I'm not sure we typically know when we are calling an ifunc.
From the gcc docs on ifunc attribute
LGTM also. I have one minor question: Is it worthwhile to add a small lit test explicitly for this? ie.
- we don't emit the '@PLT' annotation on a call when linking with static-relocation model.
- emit the annotation with PIC relocation model.
Dec 17 2019
I'm not super knowledgeable on IFuncs so I really appreciate the detailed descriptions you've provided. I'll start brushing up on ifuncs so I can review this. I have one early observation though: If its valid to tail call a non-preemptible ifunc (and since its non-preemptible I'm assuming it is tail-callable) , then saving of the toc pointer in the stub is not safe.
Dec 16 2019
Dec 13 2019
Dec 11 2019
Rebased, combined the ASM32 and OBJ32 checks that were the same into 1 ASMOBJ32 check label, moved comment on tabelgen InstrFormat member to the bh bits field.
Dec 10 2019
Dec 9 2019
- Fixed formatting in several places.
- Fixed the added Instruction format and Ins of the new pseudo op. I had implemented as if the load were a DS form instruction instead of a D form instruction.
- Collapsed the asm and obj lit testing checks together for the instructions where we don't care about the encoding (and the checks end up being the same between asm and binary output)
Dec 6 2019
- Removed all instruction encodings from the test, other then those that are expanded from the newly added pseudo instruction.
- parameterized the scratch register used in the 64-bit asm.
clang-format the changes please.
Dec 5 2019
I have a couple minor questions, but it LGTM.
Dec 4 2019
Sorry for the wait, I was on vacation. I'm going over the related pr, D70637 and this patch right now.
Addressed comments and added back missing tests.
Dec 3 2019
Rebased to pick up recent changes to PPCISelLowering.cpp
Nov 29 2019
Nov 27 2019
- Change targeted cpu in the test from pwr7 to pwr4.
- Add testing of object file generation.
- Change CHECK-NEXT directive to CHECK-DAG directive for instructions that can be scheduled differently.
- Use a file check variable instead of hard-coded register for scratch register used to move the callee address into the count register.
- Accidentally dropped the useful part of the previous suggested comment when rewording. Added it back.
- Added a local variable set to the MVT for a gpr, and replaced several conditional operator uses with it.
- Reworded another comment that made it sound like plt stubs save multiple things to the stack.
- clang-formatted the patch.
- Add back dropped comment (but reworded and expanded)
- Added a local to replace several calls to Subtarget.isPPC64()
Nov 26 2019
Only a couple minor comments. Otherwise LGTM.
Nov 21 2019
Nov 19 2019
Patch is very close. I've added a few inline comments on the test.
Nov 18 2019
Other then the few nits I mentioned LGTM.
- Various spelling and punctuation fixes.
- made 'buildCallOperands' static
- Added a comment in the 'useFunctionDescriptors' querry.
- renamed 'buildIndirectCall' and 'buildDescriptorIndirectCall' by changing 'build' to 'prepare' --> The helpers build the DAGs, setting up the call, but don't insert the call instruction itself so 'prepare' is a better description then 'build'.
LGTM. A minor suggestion: we could use a test where one of the virtual sections, say .bss is larger then the object file. Similar to X86/elf-disassemble-bss.test.
Nov 14 2019
Nov 12 2019
Nov 11 2019
A couple high level comments on the tests.
Nov 8 2019
Patch is getting pretty close, I am just going over the tests more thoroughly now. One observation for the gpr test: We have a lot of cases passing i8, but none passing i16.
Oct 30 2019
Oct 29 2019
Rebased to pick up test changes from D67125
Oct 28 2019
Oct 22 2019
Oct 21 2019
Oct 18 2019
Oct 11 2019