This is an archive of the discontinued LLVM Phabricator instance.

Fix a bug introduced by the move of AddressRanges.h into ADT.
ClosedPublic

Authored by clayborg on Jun 14 2022, 4:48 PM.

Details

Summary

The bug was introduced when the AddressRange class was no longer able to modify the End address directly and the entire range of the .text address range that contained the trailing empty symbol was replaced. There was no unit test for this, so it wasn't caught. I fixed the bug and added a unit test for it.

The effects of this bug are serious as the AddressOffsetSize in the header would be incorrectly calculated and an invalid GSYM would be created.

Diff Detail

Event Timeline

clayborg created this revision.Jun 14 2022, 4:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 14 2022, 4:48 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
clayborg requested review of this revision.Jun 14 2022, 4:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 14 2022, 4:48 PM
avl added a comment.Jun 15 2022, 2:51 AM

Thank you for catching this!

llvm/include/llvm/ADT/AddressRanges.h
30

I would suggest to leave AddressRange to be immutable.
It might prevent from side effects if borders of range would be changed.

If you think it is still better to have setStart/setEnd then we need to add assertions:

assert(Start <= End);

llvm/lib/DebugInfo/GSYM/GsymCreator.cpp
295

without setStart/setEnd we can reconstruct Address Range:

Funcs.back().Range = {Funcs.back().Range.start(), Range->end()};

This would allow not to introduce setters.

clayborg updated this revision to Diff 437265.Jun 15 2022, 11:23 AM

Removed accessors from AddressRange class to keep it immutable.

clayborg marked an inline comment as done.Jun 15 2022, 11:23 AM
avl accepted this revision.Jun 15 2022, 12:49 PM

LGTM

This revision is now accepted and ready to land.Jun 15 2022, 12:49 PM
This revision was landed with ongoing or failed builds.Jun 16 2022, 10:51 AM
This revision was automatically updated to reflect the committed changes.