Under further investigation.

Hi @MaskRay, thank you for handling this with such a clever solution.

In D152170#4425336, @peter.smith wrote:

In D152170#4425256, @kstoimenov wrote:

Looks like this or D151802 broke the ubsan ARM bot: https://lab.llvm.org/buildbot/#/builders/238/builds/3938. Could you please revert or fix?

Thanks!

It would be this one. I suspect that it is the test rather than the code change:

REGION1__PADDED_SR_SIZE = MAX(1 << REGION1__PADDED_SR_SHIFT, 32);

Where REGION1_PADDED_SR_SIZE isn't known till the second pass.

I think in the short term the test could be modified to clamp REGION1__PADDED_SR_SHIFT to an in range value. More thought would need to be given to whether LLD can protect itself against out of range shifts coming from scripts. I expect this could get fixed within a day assuming the author or MaskRay is around, it is the end of the day here and I need to leave in 5 minutes so I'm not in a good place to take action myself.

run git-clang-format

In D151802#4419200, @MaskRay wrote:

Since you do not use arc diff 'HEAD^' to update the differential, arc patch D151802 doesn't give author name/email information. You need to provide a git commit --amend --author='...' command for me to give credits to you!

Some background related to this proposal:

If there are no more comments, can you kindly commit this change, when you have time @MaskRay ?

If there are no more comments, can you kindly commit this change, when you have time @MaskRay ?

All MaskRay's comments were addressed.

All MaskRay's comments were addressed.

MaskRay' s comment about the test was addressed.

In D152170#4403739, @peter.smith wrote:

Thanks for the updates. I've made a small suggestion to simplify the code.

All comments were addressed.

In D152170#4399685, @peter.smith wrote:

Thanks for the update. The code changes look reasonable to me. I'd ideally like to give a suggestion on how to simplify the test but I'm a bit short on time at the moment. Will try to come back soon. With luck one of the other reviewers will have a suggestion.

*The code was improved.
*The test was simplified.

All comments from MaskRay were addressed.

In D152170#4396147, @peter.smith wrote:

May I suggest an alternative approach? Instead of checking the memory regions when assigning addresses, move the check to its own function, something like verifyMemoryRegions() this could be called once after finalizeAddressDependent Content. It would require fewer changes to the code and would be working on final addresses.

One possible disadvantage of this approach, but I think that they are likely shared with suppressing the errors on the first pass, is that the contents of a memory region could spill into the next one. This could lead to overlapping addresses which may cause a different, more difficult to understand, error message. I don't think that this will be the case today, but is a possibility.

Comments were addressed.

In D151802#4390984, @peter.smith wrote:

Thanks for the update this looks good to me. If in a later change we can make the ByteCommands set the SHF_ALLOC flag on the OutputSection we can update the test.

Please leave a few days for MaskRay (Code Owner) to comment to see if he has any suggestions.

All current suggestions addressed:

In D151802#4384444, @peter.smith wrote:

Just so I understand I think the main use case for this is where there is a linker script with OutputSections assigned to memory regions that may or may not contain input sections, it will depend on the input sections. As they won't get input sections they won't get the SHF_ALLOC flag and will get a spurious warning?

I agree that we shoudn't warn in that case, although I think the proposed change might suppress too many warnings at the moment.

I note that GNU ld permits the assignment of non SHF_ALLOC sections to MEM regions, assigning them an address if SHF_ALLOC, (although it ignores the empty ones). Although that is a separate issue.

The patch was updated to the main branch.

In D140302#4004906, @arsenm wrote:

Where are these coming from? I thought we directly emitted pointer constants now?