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.

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).

In D72197#1805438, @bd1976llvm wrote:

I wonder if is this really a bug or intentional. Clang and LLVM seem to ignore the possibility of symbol interposition (see: -fno-semantic-interposition). What is the LLVM policy for this?

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:

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.

LGTM.

Uploaded wrong diff last time.

In D71504#1787912, @hubert.reinterpretcast wrote:

In D71504#1787758, @ZarkoCA wrote:

I don't know what happens in the case where the front-end doesn't handle some cases correctly but the back-end does. Is there a danger in enabling this in the back-end knowing we don't get correct IR for complex types for AIX? Is there an error in the front-end for those before we get to this step?

There's no error from the front-end. In terms of continuous delivery, this leads to silently bad code that could lead to incorrect output or crashes at runtime.

Rebased and added '-verify-machineinstrs` to the run commands.

In D71649#1792275, @MaskRay wrote:

🤡🤡🤡(Any chance this is reviewed before I watch star wars? (I felt that if I did not say, due to vacation schedules, this might took a lot longer..))

In D70570#1789789, @jhibbits wrote:

Update diff. It would help if I actually compiled when rebasing...

@sfertile I figured the available-externally.ll test would suffice for this.
Do you think a separate test is needed, or a comment is warranted on this
test for it?

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.

1. we don't emit the '@PLT' annotation on a call when linking with static-relocation model.
2. emit the annotation with PIC relocation model.

In D70570#1778180, @jhibbits wrote:

In D70570#1778165, @sfertile wrote:

In D70570#1777864, @jhibbits wrote:

• It is an error to not use a PLTREL relocation for a call that needs to be indirected through the PLT. The linker will complain to recompile with -fPIC.

Means you can't link static execs against shared objects anymore. Is this the intent? Or am I missing something obvious?

My phrasing wasn't quite right, I guess.
The linker will create the PLT stubs by nature of it being an external shared object, or something to that effect. What I really meant was that it's an error to need a PLT and not have it (linker will warn or error if you try to create a .so and use anything other than PLTREL relocations).

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.

In D71504#1786135, @hubert.reinterpretcast wrote:

In D71504#1785698, @ZarkoCA wrote:

So it doesn't look like we need to add any special handling for struct returns on AIX? I had a quick look and none of the other PPC targets seem to have anything.

By the time we get the IR in this form from the front-end, we shouldn't need special handling here. We know the front-end does not represent complex types in an ideal way for AIX here.

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.

@MaskRay , @jhibbits Thank you for providing the correct ABI doc, and for the answers. I'll start going through it.

In D70570#1758956, @Bdragon28 wrote:

This doesn't seem quite sufficient for FreeBSD kernel modules. It's emitting R_PPC_REL24 instead of R_PPC_ADDR16_HA/R_PPC_ADDR16_LO pairs for stuff like memset and memcpy,
which are unusable because we're running modules in KVA and the kernel text in the 32 bit DMAP. Still need to figure out why these are being emitted this way when using freestanding.

Hi Justin,

• 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)

I believe this commit breaks a number of the build bots. eg: Arm , PowerPC

In D70724#1768155, @hubert.reinterpretcast wrote:

Just noting here that the test does not actually enforce that r2 is saved before being written to. Similarly for the other tests.

• 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.

I have a couple minor questions, but it LGTM.

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.

Rebased.

Rebased to pick up recent changes to PPCISelLowering.cpp

• 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.

In D70126#1759491, @Xiangling_L wrote:

Since this is a NFC patch, does it make sense to add NFC on patch title?

• clang-formatted the patch.
• Add back dropped comment (but reworded and expanded)
• Added a local to replace several calls to Subtarget.isPPC64()

Only a couple minor comments. Otherwise LGTM.

Ping.

Patch is very close. I've added a few inline comments on the test.

Other then the few nits I mentioned LGTM.

In D70126#1749884, @cebowleratibm wrote:

The new logic is much clearer, though it's hard to discern that the semantics remain correct because so much logic was redistributed. If a series of smaller commits were possible it would be easier to review.

• 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.

A couple high level comments on the tests.

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.

In D67125#1725439, @daltenty wrote:

In D67125#1725332, @daltenty wrote:

I don't think that initialized global data should be coming out as label symbols (everything appears to be contained in a ".data" csect). As I understand it only static data should be glob'd into one big csect, as per the XCOFF spec "An initialized definition for a global data scalar or structure ... is contained in its own csect". So each of these globals should have their own csect, which is the behavior exhibited by XLC.

I've observed that GCC on AIX is currently exhibiting this initalized-globals-in-one-csect behavior, but as we might expect, it interferes with the linkers ability to garbage collect unneeded global data.

I believe this is also an issue in the assembly path in D66154

As per out of band discussion, we will revisit this behavior under an option flag in a future revision and adopt the GCC behavior for now.

Rebased to pick up test changes from D67125

In D69528#1724277, @jasonliu wrote:

This patch is going to have conflict with D67125. Depending on which patch is going to go land first, the test case is likely need to get modified.

I would propose to let D67125 be the parent and add object file generation test in this patch.

LGTM.

LGTM.