- User Since
- May 29 2017, 8:02 AM (203 w, 1 d)
Thu, Apr 15
Was able to reproduce the issue that caused the left shift of -1.
Fixed the patch by replacing the shift with a multiply.
Also realized that the type should have been signed and not unsigned.
Wed, Apr 14
Thank you for the explanation!
Mon, Apr 12
Reopening review to investigate the issue.
Fri, Apr 9
Wed, Apr 7
Mon, Apr 5
Updated test case to include -O0.
Wed, Mar 31
Tue, Mar 30
Removed extra whitespace.
Updated to always add the header to the got on PPC64.
Thu, Mar 25
I'll need to go over this again to refresh my memory. As a quick recap though:
- On PPC, if we create a .got, then the first entry must be the tocbase.
- If .TOC. is not defined by one of the input objects, then we do not increment the number of .got entries here
- If we synthetically define .TOC. in the linker, then we end up with a .got section in some cases where we would otherwise not need one.
Rebased the patch to the latest top of trunk and added the test case.
Wed, Mar 24
Looking at the test case changes there seem to be a few places where we are not generating expected code.
I've added comments to some of those places.
Tue, Mar 23
I would like to restart the discussion on this patch.
I understand that the design of this fix is not ideal however I'm not sure of a better solution to this.
Adding the GOT to all linked binaries seems like too much. However, using hasGotOffRel isn't going to cover all of the test cases as it will not be set in all of the cases. The code needs to add a first symbol to the GOT in all cases where we use a GOT but we don't want to add a GOT if nothing else goes into it.
Mon, Mar 22
Thank you for the review!
Mar 17 2021
Mar 16 2021
I think I may have misunderstood your initial comment.
I don't think that the ABI has an issue with the code pattern:
pld 3, <Immediate Value>(0), 1 add 4, 3, 13
The ABI does not explicitly state that those registers must be equal. It also does not say explicitly that they can differ so I don't think there is a clear direction from that perspective. Logically it would make sense for those registers to be able to differ if they are used in multiple places or if they must be used across function calls (as in the example I added).
Linking the object files generated by LLVM with ld produces the correct result with the mr and not the nop. Therefore it seems that ld expects to have to handle this kind of code. While this situation will not arise very often I believe that this is something LLD should be able to handle.
Added full test case that was compiled from C source to show the issue.
Mar 15 2021
Mar 12 2021
Thank you for adding this!
Other than one minor nit I think this LGTM.
Mar 11 2021
Rebased patch to prepare for commit.
Mar 1 2021
Comments relate to just cleaning up the patch a little.
Feb 22 2021
I'm sorry, I should have added the full test case.
Feb 18 2021
Fixed spelling mistake.
Feb 17 2021
Added missing FeatureROPProtection to P8AdditionalFeatures.
I gave this patch a try and I can't see any concerns with it.
Feb 16 2021
I just have one final nit otherwise LGTM. Feel free to fix that on commit.
Please wait a couple of days and see if @MaskRay has any further comments.
Feb 11 2021
Feb 4 2021
Just nits this time around.
Feb 3 2021
Rebased the patch to the top of main.
This feature has already been covered by https://reviews.llvm.org/D77448
I am going to abandon this review.
Feb 2 2021
Adjusted test cast that I forgot to fix on rebase.
Rebased to top of main branch.
Feb 1 2021
I'm sorry @MaskRay I don't want to keep bugging you but I really don't understand what you meant by your comment with respect to GotPltSection::hasGotPltOffset.
Updated incorrect comment in test case.
There is one more thing that does not look quite right. It may be an encoding problem for one of the instructions. I found it in the test but you will have to trace it back to the place where it is generated in order to fix it.
Jan 27 2021
I have a few comments. Most of them are nits but there is a functional issue as well.
Jan 25 2021
Jan 22 2021
Jan 7 2021
Fixed spelling mistake in comment.
Jan 6 2021
Updated some of the test cases for the 32 bit compilation.
Added the todo comment in the TD file.
Jan 5 2021
Added comment for function.
Fixed variable name.
Added test case for 32 bit target.
Since D92959 is now in I am going to abandon this patch.
Forgot to delete the old lines in the TD file.