[DEBUGINFO] Add flag for DWARF2 or less to use sections as references.
ClosedPublic

Authored by ABataev on Feb 22 2018, 9:14 AM.

Details

Summary

Some targets does not support labels inside debug sections, but support
references in form section +|- offset. Patch adds initial support
for this. Also, this patch disables emission of all additional debug

sections that may have labels inside of it (like pub sections and
string tables).

Diff Detail

Repository
rL LLVM
ABataev created this revision.Feb 22 2018, 9:14 AM
probinson accepted this revision.Feb 27 2018, 9:57 AM

LGTM with minor changes.
I didn't try to verify that you found all the places where this check would be needed; I trust that you will trip over them if you missed any. :-)

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
128 ↗(On Diff #135440)

I'd prefer "section+offset" as I doubt you would ever have a reference to something before the start of a section!

320 ↗(On Diff #135440)

s/DWARF2 or less/DWARF v2/
i.e. use the preferred version spelling, and there is no "less."
The <= 2 below could also be simply == 2 but that's up to you.

test/DebugInfo/X86/sections_as_references.ll
6 ↗(On Diff #135440)

Do you really need to check for .file directives?

26 ↗(On Diff #135440)

I think the exact offset is not important? Just that there is one. If that's the case, then using a regex for the offset will be less sensitive to irrelevant changes in emitting debug info.

This revision is now accepted and ready to land.Feb 27 2018, 9:57 AM
JDevlieghere added inline comments.Feb 27 2018, 1:15 PM
lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
274 ↗(On Diff #135440)

The LLVM coding standard says not to put braces around single statement blocks.

ABataev added inline comments.Feb 27 2018, 1:22 PM
lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
274 ↗(On Diff #135440)

Only if there is no else branch with multiple line substatement. In this case, both substatements must be enclosed in braces.

JDevlieghere added inline comments.Feb 28 2018, 2:47 AM
lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
274 ↗(On Diff #135440)

But that's not the case here, is it?

ABataev added inline comments.Feb 28 2018, 6:29 AM
lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
274 ↗(On Diff #135440)

Why not? The else substatement is 2-line.

ABataev marked 2 inline comments as done.Feb 28 2018, 6:36 AM
ABataev added inline comments.
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
128 ↗(On Diff #135440)

Ok, will do

test/DebugInfo/X86/sections_as_references.ll
6 ↗(On Diff #135440)

No, will remove these checks

This revision was automatically updated to reflect the committed changes.

Using sections as references is, in general, a good idea when we can so I'd rather not tie that to whether or not we emit certain sections. If you don't mind reverting that aspect of the change I'd appreciate it and we can get that in via some other preference or a strict requirement via the asm printer as we're planning on doing for the rest of the nvptx section emission.

Using sections as references is, in general, a good idea when we can so I'd rather not tie that to whether or not we emit certain sections. If you don't mind reverting that aspect of the change I'd appreciate it and we can get that in via some other preference or a strict requirement via the asm printer as we're planning on doing for the rest of the nvptx section emission.

These sections have labels inside of them + they are not supported by PTX format. Ok, I will revert the patch.

I really hate to go on about this, but the truth is that I've been told this so many times that it instantly caught my eye. 😀

lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
274 ↗(On Diff #135440)

It's still a single statement, only it that happens to be split across two lines, which doesn't affect whether we need braces.

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1580 ↗(On Diff #135440)

Same here

1584 ↗(On Diff #135440)

And here

I really hate to go on about this, but the truth is that I've been told this so many times that it instantly caught my eye. 😀

  1. Several years ago I got a lot of reviews where reviewers insisted(!) to enclose multiline (not multistatement!) single-statement substatements into braces. I did for the last 6 years and had no problems with it. This is the first time I see this. Di I miss something, the rules changed already?
  2. There is a patch D26943, that was accepted but not committed to enclosing multiline (not multistatement) substatements in if and for statements. It is clear that multistatement substatements must be enclosed and this kind of rule is not required at all.

I'm not against this kind of change but seems to me this is not strictly standardized.

In fact none of this is in the style guide itself, it's just lore and the
usual argument for consistency with surrounding code.

There are many examples of most points on this spectrum - the generally
agreed upon bit is "single line blocks (without connected/related blocks -
like else, etc) shouldn't have braces". Whether you brace multiline, single
statement blocks, or brace all related blocks if one of them is braced, is
much more variable and I generally leave up to the author unless there's
some really clear consistency argument to be made.