This change makes good sense to me, as I suggested pretty much the same thing 2 years ago (D38554). In the end for us it was easier to fix LLD and switch to GNU ABI ignoring IFUNC support entirely than to convince the others and support the change. Yet either way believe that it is logical not to emit PLT for static memory model. Personally I can accept this, but I wonder whether there are more comments coming.

Actually thanks for adding C checks too. The new revision also looks good to me.

Thanks.

I noticed a typo in the file, but the rest looks good now.

Yes, that's the point, since 8548 is an alias.

@jhibbits Should not the test in test/Preprocessor/init.c also ensure that __SPE__ and __NO_FPRS__ macros are defined with CPU 8548? The rest looks good to me, thank you.

@jhibbits, thank you for merging. Will we have this in LLVM 9.0?

Not sure whether I understood you, do not you already have the common switch by feature, named spe, in Features["spe"] line for that?

@ruiu, we have some hardware, which bootloader does not support PT_GNU_STACK segments, and would therefore benefit quite a bit from this option. As this does not really depend on NetBSD support, could you please merge this patch in time for 9.0? Thanks!

Could somebody commit it into 9.0? Quite a lot of people depend on this option including us. Since it got accepted I do not see a reason for this to get postponed any further. Thanks!

Right, ok. I have been testing this for quite some time now, including maths, and so far had no issues. Can this get merged into 9.0? I do not believe there are enough obstacles to postpone it any further. Thanks!

@jhibbits @kthomsen Sorry for the delay, I checked the change in PPCISelDAGToDAG.cpp, and it indeed fixes the issue. So far I ran into no other bugs and have no objection of this getting merged.

@nemanjai, sorry, under merging I meant committing into llvm upstream.

This is a series of patches, which I believe should merged altogether. Currently the following patches are relevant:

@jhibbits, @kthomsen, it appears that current patchset has issues when handling && on me. I have it applied over llvm 8.0.0 rc2, and the following code returns 0 to me with -O2 and below:

Thanks a lot!

I am fine to create bugs, but I am yet to ask for commit access, and somebody needs to commit the patch for the time being. For this reason I thought it would be faster to leave the rest up to you.

Done, I must have been quite tired yesterday, thought I included it.

Done with the test update. As for your other comments, there are some minor differences in section handling for different output types in GNU binutils, but I believe they have little to do with LLD abilities as of now. ppclinux one is chosen when clang is used for linking under Linux target. You can get some more details by checking ld/emulation folder, but I believe it is just fine to have these as synonyms for a long while to let most of the code continue building just fine.

Sure, superseded by D58005.

Sorry, did not realise that we have tests for this functionality in such an undescriptive place. Added tests and merged with D58007 as suggested by Rui.

That's pretty good. Do you think it is possible to land it in 8.0 release? @hfinkel?

Thanks for pointing it out. You could use hasFeature from there during construction:

return SetCGInfo(
        new PPC32TargetCodeGenInfo(Types, CodeGenOpts.FloatABI == "soft" || getTarget().hasFeature("spe")));

It works for me, but probably a separate argument is best to be used (or at least the current one is to be renamed).

Right, I noticed the same thing yesterday. There is an override calling LowerVAARG for 64-bit integers, yet that is not a thing that does lowering for all the rest. I believe it is derived somewhere from td calling conventions. I will check it out later this evening and mail you if I find anything (vit9696 at avp dot su).

@jhibbits it appears that va_list is not functional with SPE (va_arg returns garbage for double and stuff like printf is not functional). Is this expected or I miss a patch?

In D54583#1368816, @jhibbits wrote:

Hi @vit9696, it looks like Kei's patch fixes the issue you're seeing as well.

Hmm, I have not yet tried to explore this, but I get a feeling a regression appeared somewhere during the patch iterations. Either this or D54409.
At this point I am consistently getting weird generated instructions for __floatundidf from compiler-rt (compiling with freebsd & -O3), yet I have a correct one in my files.
What makes it strange is that the logs show that the correct and first incorrect examples were generated by the same compiler (binary file) with the same flags, yet I no longer can get the correct one.

Justin, I am currently testing the latest patches, yet so far it looks very good. Thanks.
I rechecked GCC, and it seems that it forces 64-bit long double for SPE regardless of the target.
Could you please force that in LLVM as well? While imperfect, I currently changed the following part of code in clang/lib/Basic/Targets/PPC.cpp, and it seems to work for the needs:

You are right, had to modify it like this to get the crash with FreeBSD triple:

Actually I am not sure about Linux, since this is bare metal, and I just used what fited. However, it does not look like 128-bit or 64-bit long doubles are related.
I retried to compile the test case with powerpc-gnu-freebsd target (and even made a compile-time assertion to ensure that long double is now 64-bit), yet it still crashed in a similar way.
Might it be another LLVM 7 vs LLVM 8 difference? Does it crash for you with Linux target?

@jhibbits sorry for not answering earlier, I was occupied with NY holidays, and then had a lot of stuff on the road.

Ok, I found the fix for the first crash that landed in 8.0 trunk. It works fine for me if backported to 7.0.1:
https://reviews.llvm.org/D50461

In D49754#1321078, @glaubitz wrote:

I have applied this patch to the llvm-toolchain-7 package in Debian and did not see any regressions on x86_64 or 32-Bit PowerPC. Additionally, I have included the patches from https://reviews.llvm.org/D54409 and https://reviews.llvm.org/D54583 saw no regressions on x86_64 and 32-bit PowerPC.

All three patches will be part of the next upload of the llvm-toolchain-7 package in Debian unstable which will be version 1:7.0.1~+rc2-9.

Thanks for the fix. I made a quick check of the mentioned patch, and it looks like it does solve the issue. However, besides the previous crash, which remains unfixed as of 7.0.1rc2, there is another instruction selection failure crash that may be caused by the change. I have not yet had a chance to properly research it, but here is an example if you feel interested: http://llvm.org/svn/llvm-project/compiler-rt/tags/RELEASE_701/rc2/lib/builtins/divdc3.c

Thank you for working this. I tried the change and have a couple of suggestions:

1. -mspe option in GCC works like -mspe=yes or -mspe=no. While it does make sense to have it the way you did (-mno-spe and -mspe) it would be nice to have at least have an alias for compiler compatibility.
2. One of the known CPU examples with SPE support is MPC8548 (https://www.nxp.com/docs/en/reference-manual/MPC8548ERM.pdf), it will be nice to have it recognised in -mcpu argument as 8548 like in GCC.

The proposed change seems right to me in both senses:

• moving the invariant out of the loop (obvious);
• not making a reciprocal here, as it may add an extra overhead for non-constant divisors.

Now that https://reviews.llvm.org/D44926 was merged upstream, may I request this to be merged as well?
As stated above it is pretty much pointless to have the check doubled, and after more than a week nobody thought of any issue.
Thanks :)

Morning. Ok, it looks like I hardly checked the IR and a ton of passes were left unnoticed making no sense of the tests :D. Sorry, fixed.

Hm, I did run tests before uploading the diff and they passed. Perhaps I upped the wrong file. I will recheck tomorrow and resubmit.

Thanks for the review. We are good now, I suppose. Could you merge it for me please?

Thank you too!

Done, could you merge it please?

Comments? But nobody reeeeads them anyway xD. Updated, kind of forgot about them.

Thank you for such an involved explanation! It is pretty clear now I think. I applied all the mentioned changes and updated the diff.
In case there are no other issues may I ask you to commit this?

Thanks for a quick reply. I think your comments were less helpful, because we had different assumptions, and you ended up explaining fairly trivial things leaving the ones I asked about unanswered. Apparently, assuming you got it right, I indeed misunderstood what James suggested. However, that's less important and easy to change.

Well, I found some time to prototype what we discussed. As I got to the code I think I discovered more hidden issues that were not taken into account, and mentioned them as the comments in the code.

Well, ok, I think it might actually work this way. I will try to find some time in the next few days to update the patch.

It is not that simple, because the size of the header part should be aligned to the page size (from what I remember at least). And we cannot know the alignment rules / page sizes of the target system. We could try checking the subsequent segments, but it is not so nice. I think that unless we replicate the area used by the header and the subsequent padding, the resulting ELF may become unloadable.

I think I could have missed the point of the description a little, but I know of at least a total of 3 cases regarding ELF header:

• ELF header is present in PT_LOAD segment
• ELF header is mentioned in PT_PHDR segment
• ELF header is not mentioned or present anywhere, but the segment data starts at a random offset the linker decided to use.

I am personally not sure if it is the correct solution, because I do not understand why a segment with 0 filesize and a 0 offset should corrupt an ELF while copying regardless of its type.
I would personally prefer special-casing two segments to writing a check against both MemSize and FileSize, but it does not feel nice to me.
More importantly, if you have a PT_LOAD "marker" segment with all zero values:

• will gnu binutils objcopy also corrupt such an ELF?
• will a Linux/BSD-based system load such an ELF?

If in both cases the answer is no, then we should probably just special-case indeed, but I still cannot say that silently overwriting the header is any good. Perhaps we should print an error and abort?

@jhenderson, thanks, NOBITS makes a lot more sense to me than your previous example. Yet I am unaware of any software or even any tool that would rely on such on offset, and to my eyes the linkers are simply doing the wrong thing by not setting the offset of 0 for whatever reason. In my opinion being able to copy more ELF files, which will be most likely valid and loaded on a target system, is more important, than trying to support some shady offset to a non-existent memory nobody knows how to calculate.

@jhenderson, sorry for my ignorance, but I miss the point of your .bss example just a second time. Could you elaborate a little more?

Thanks for the comments!

This was committed in r323366, thanks to @rafael!

Updated with suggested test extension by Peter Smith.

Added a test case and restyled the check like Igor suggested.

In D42471#986333, @peter.smith wrote:

Thanks for sending the patch. What I'd like to know first is in what circumstances the synthetic section has no parent and is this expected to be possible? If it isn't expected to be possible then it would be better to fix that and not try to account for the no parent case. The implementation has changed quite a bit since I last looked so I'm not sure of the intention.

Do you have a test case that you could add to your patch? For example find a case that currently faults without the patch and check that it links correctly with it. Perhaps checking that the output has no .ARM.exidx sections present.

This got merged, thx to tnorthover.

This got merged, thx to tnorthover.

Yes, I actually meant to ask someone to commit this for me. Given the timespan I thought it was evident :)

Ping, could you commit this please?

Ping

@hfinkel, thanks for a reminder, somehow forgot about it. Added now :)

Well, the more I think of it, more strange it feels. I indeed was not aware of GNU_IFUNC and kind of expected there to be no difference between EABI and GNU ABI in this sense. While it does not make any sense to emit PLTREL relocations for EABI, it is a must for GNU_IFUNC support and thus there are very few ways (aside -fno-plt) not to emit PLT calls. Hacking in an abi check in the place relocation model check will not be nice. For this reason, once -fno-plt lands and works with compiler-rt calls I will update this with an additional check that enables -fno-plt in EABI by default, which should be sane and more reliable.

Removed the unnecessary codegen patterns.

@ruiu, alright, rewrote it more explicitly.

Thanks, updated the patch.

@hfinkel, that sounded like a good idea in the beginning, but after I implemented it by changing the condition to the following:

So, am I right that the correct approach is to add an additional check whether eabi target is used?
Another way is to ensure that lld handles PLT relocations and solves them as relative relocations if possible.
I think that both changes should be made regardless of the reasons behind, but I would like to hear the opinion of @ruiu and @davide from lld project first.

Alright, now I understand that the condition I added in the beginning did not conform to all the other relocation models available in LLVM:

-relocation-model                                 - Choose relocation model
  =static                                         -   Non-relocatable code
  =pic                                            -   Fully relocatable, position independent code
  =dynamic-no-pic                                 -   Relocatable external references, non-relocatable code
  =ropi                                           -   Code and read-only data relocatable, accessed PC-relative
  =rwpi                                           -   Read-write data relocatable, accessed relative to static base
  =ropi-rwpi                                      -   Combination of ropi and rwpi

Initially I added PLT to be enabled only for PIC model, and you are right, in case of dynamic-no-pic model (which allows dynamic relocation) PLT must be used. I updated the diff to enable PLT unless static relocation model is used. No issues from then on?

Right, I could finally understand your opinion and perhaps even the reasons behind it. However, I cannot agree with them due to the following:
— a working linker you describe is something subjective, in my opinion it is a linker capable of translating relocations defined by a standard or a specification, not necessarily all, and efficiency is not a necessity.
— the assembler indeed does not know what binding should be used, however, the extension to the compiler is incorrect. The compiler, which is responsible for emitting the relocations, is aware of the relocation model, and thus is expected to emit relocations suitable for this model. Since PLT is a PIC-specific feature, which is known to the compiler, the compiler should decide whether to generate PLT relocations or not.
— while you state that two relocation models is a historic artifact, LLVM currently implements a check whether to use PLT or not, so if this code is here, then it should work properly, otherwise it is a bug.
— your statement regarding more common GNU Power-PC ABIs being towards generating position independent may be correct, however, it has nothing to do with embedded ABI we are using here. Furthermore, if the "working" linker generates PLT relocations for a static model, it will simply crash a loader that has no support for PLT.

@joerg, this is nonsense in general, and possibly implementation dependent. A simple example as follows:

@joerg, please read carefully the comment @hfinkel posted (https://reviews.llvm.org/D38554#889007) in case we cannot understand each other.

@joerg, while it is a valid claim that LLD could be implementing PLT reference support, it is not sane to claim that they should be used with STATIC relocation model. Please refer to what gcc does here and what @hfinkel said.

It has been over two weeks since the last comment, and no explanations why this could cause issues. Given no response from @joerg I consider his claims invalid, and ask this to be approved and merged into trunk.

Ping, would anybody commit this please?

In D38509#895496, @davide wrote:

Was this ever committed?

In D38554#888873, @joerg wrote:

In that case: the short version is that there is no EABI support at the moment. That patch changes the behavior for SYSV ABI, it is not acceptable as such. The test case doesn't reflect the ABI variance either.

In D38554#888873, @joerg wrote:

In that case: the short version is that there is no EABI support at the moment. That patch changes the behavior for SYSV ABI, it is not acceptable as such. The test case doesn't reflect the ABI variance either.

In D38554#888793, @joerg wrote:

Let's start with the obvious: I have no idea why you need this code at all. NetBSD builds a mix of static, PIC and PIE code using GNU ld without hitting any unsupported relocations. As I said before, the normal ABI on PowerPC is mostly PIC by default. I wonder if the root of your problem is that you want EABI and not the SYSV ABI.

In D38554#888745, @joerg wrote:

This doesn't look right to me at all. The normal SYSV ABI on PowerPC is effectively PIC, with a few edge cases. This really looks like a step backwards.

Alright, fixed the line width as well.

In D30819#710056, @hfinkel wrote:

A comment on the variable names; otherwise I think this looks good. Thanks!