User Details
- User Since
- Mar 10 2017, 4:36 AM (277 w, 4 d)
May 2 2022
Got it. To be honest, I cannot imagine a situation where one would prefer to keep separate behaviour, and with GCC it would be exactly just one. In my view, if we introduce -fprofile-prefix-map, it should rather mirror the entire -fcoverage-prefix-map behaviour, not some parts of it.
Jan 10 2022
Tested this on what we had on 13.x branch. Works fine. Thank you very much.
Nov 28 2021
This changeset causes a regression mentioned in https://lists.llvm.org/pipermail/llvm-dev/2021-November/153999.html.
May 27 2021
Nov 29 2019
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.
Nov 12 2019
Actually thanks for adding C checks too. The new revision also looks good to me.
Oct 28 2019
Thanks.
Oct 26 2019
I noticed a typo in the file, but the rest looks good now.
Oct 23 2019
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.
Sep 5 2019
@jhibbits, thank you for merging. Will we have this in LLVM 9.0?
Jun 29 2019
Not sure whether I understood you, do not you already have the common switch by feature, named spe, in Features["spe"] line for that?
Jun 28 2019
@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!
Mar 31 2019
@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.
Feb 20 2019
@nemanjai, sorry, under merging I meant committing into llvm upstream.
Feb 19 2019
This is a series of patches, which I believe should merged altogether. Currently the following patches are relevant:
Feb 18 2019
Feb 12 2019
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.
Feb 11 2019
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.
Feb 9 2019
Jan 29 2019
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).
Jan 28 2019
@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?
Jan 24 2019
Jan 23 2019
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.
Jan 20 2019
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:
Jan 17 2019
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.
Dec 21 2018
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
Dec 7 2018
Nov 16 2018
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
Jul 31 2018
Thank you for working this. I tried the change and have a couple of suggestions:
- -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.
- 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.
Jun 22 2018
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.
Apr 4 2018
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 :)
Mar 28 2018
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?
Mar 27 2018
Feb 15 2018
Thank you too!
Feb 14 2018
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?
Feb 13 2018
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.
Feb 9 2018
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.
Feb 8 2018
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?
Feb 7 2018
@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!
Feb 2 2018
Jan 24 2018
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.
Dec 10 2017
This got merged, thx to tnorthover.
This got merged, thx to tnorthover.
Nov 29 2017
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?
Oct 26 2017
@hfinkel, thanks for a reminder, somehow forgot about it. Added now :)
Oct 25 2017
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.
Oct 24 2017
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:
Oct 23 2017
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?
Oct 22 2017
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.
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?