Page MenuHomePhabricator

ELFAsmParser: Remove non-SHF_ALLOC or non-executable sections' line info/address ranges contribution for -g
ClosedPublic

Authored by MaskRay on Sun, Nov 15, 10:32 AM.

Details

Summary

I filed the issue https://sourceware.org/bugzilla/show_bug.cgi?id=26850 ,
which was acknowledged and fixed in GNU binutils 2.36

This patch adds the similar behavior to MC.

Diff Detail

Event Timeline

MaskRay created this revision.Sun, Nov 15, 10:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptSun, Nov 15, 10:32 AM
MaskRay requested review of this revision.Sun, Nov 15, 10:32 AM

The new behavior seems reasonable and matches GNU as 2.36 https://sourceware.org/bugzilla/show_bug.cgi?id=26850

Probably worth including a bit more detail from that bug (it's useful to know that you filed/fixed that bug - not that that discredits the work, but clarifies that the sentiment (while endorsed by the bug/patches being approved by other members of the compiler community) was initially expressed/motivated/brought up by you there as well). Maybe something like "My patch to implement similar functionality in GNU binutils was accepted and released in GNU binutils 2.36" or the like might be suitable.

Does this LLVM patch include the same generalization discussed in the GNU Binutils bug ( https://sourceware.org/bugzilla/show_bug.cgi?id=26850#c2 ) regarding non-loaded sections?

llvm/test/MC/ARM/dwarf-asm-multiple-sections.s
79–85

Why the change to DWARFv3 here?

MaskRay marked an inline comment as done.Mon, Nov 16, 4:33 PM
MaskRay added inline comments.
llvm/test/MC/ARM/dwarf-asm-multiple-sections.s
79–85

DWARF v3 and v4 use the same format. I think picking a check prefix which represents the lowest standard that something is implemented is reasonable.

dblaikie added inline comments.Mon, Nov 16, 4:34 PM
llvm/test/MC/ARM/dwarf-asm-multiple-sections.s
79–85

Ah, sure - might be better suited as a separate patch, though?

MaskRay updated this revision to Diff 305625.Mon, Nov 16, 4:41 PM
MaskRay marked an inline comment as done.
MaskRay retitled this revision from ELFAsmParser: Remove non-executable sections' line info/address ranges contribution for -g to ELFAsmParser: Remove non-SHF_ALLOC or non-executable sections' line info/address ranges contribution for -g.
MaskRay edited the summary of this revision. (Show Details)

Does this LLVM patch include the same generalization discussed in the GNU Binutils bug ( https://sourceware.org/bugzilla/show_bug.cgi?id=26850#c2 ) regarding non-loaded sections?

Thanks for spotting this! (I struggled whether SHF_EXECINSTR should imply SHF_ALLOC)
Since neither GNU as nor MC disallows "x", I assume it is meaningful and check the SHF_ALLOC flag.

MaskRay updated this revision to Diff 305627.Mon, Nov 16, 4:48 PM
MaskRay marked an inline comment as done.

Pre-commit improvement to the test file

dblaikie accepted this revision.Mon, Nov 16, 7:07 PM

Sounds good to me (I guess ALLOC and EXECINSTR can both be enabled separately? Is this consistent with the Binutils change? (or does one subsume the other?))

This revision is now accepted and ready to land.Mon, Nov 16, 7:07 PM

Sounds good to me (I guess ALLOC and EXECINSTR can both be enabled separately? Is this consistent with the Binutils change? (or does one subsume the other?))

Thanks for the review!

This is consistent with the GNU as change: only "ax" generates an address range.

This revision was landed with ongoing or failed builds.Mon, Nov 16, 8:02 PM
This revision was automatically updated to reflect the committed changes.