Page MenuHomePhabricator

[DEBUGINFO] Add support for emission of the inlined strings.
ClosedPublic

Authored by ABataev on Feb 16 2018, 8:03 AM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

ABataev created this revision.Feb 16 2018, 8:03 AM
probinson added inline comments.
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
118 ↗(On Diff #134625)

Are you intending to add platform-specific defaulting within DwarfDebug? If not, then this is a simple on/off switch that defaults to off.

test/DebugInfo/Generic/inlined-strings.ll
38 ↗(On Diff #134625)

This is spelled incorrectly (I expect you want DW_FORM_strp), and only verifies that the form does not appear after the last positive matching string.

I would be more inclined to run the test twice, once with Enable and once with Disable, with checks along the lines of

ENABLE-NOT: DW_FORM_str{{(p|x)}}
ENABLE: DW_FORM_string
ENABLE-NOT: DW_FORM_str{{(p|x)}}

DISABLE-NOT: DW_FORM_string
DISABLE: DW_FORM_str{{p|x)}}
DISABLE-NOT: DW_FORM_string

That makes the test specific to the behavior of the option, and the forms it produces, omitting irrelevant concerns about tags and attributes.

ABataev added inline comments.Feb 16 2018, 11:11 AM
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
118 ↗(On Diff #134625)

Yes, this is what Eric Christofer suggested to do as the first step. I'll enable it for NVPTX target

test/DebugInfo/Generic/inlined-strings.ll
38 ↗(On Diff #134625)
  1. I don't care about some particular form here, I'm just checking that there are no any strings at all, in any form.
  2. There are a lot of tests already where this new option is disabled by default and they have checks for DW_form_strp and other forms.
probinson added inline comments.Feb 16 2018, 11:18 AM
test/DebugInfo/Generic/inlined-strings.ll
38 ↗(On Diff #134625)
  1. Actually you want to specifically verify that you have at least one string, that it uses DW_FORM_string, and that no strings use any of the indirect forms.
  2. Fair. In that case, just do:
CHECK-NOT: DW_FORM_str{{(p|x)}}
CHECK: DW_FORM_string
CHECK-NOT: DW_FORM_str{{(p|x)}}
ABataev updated this revision to Diff 134667.Feb 16 2018, 11:36 AM

Improved test checks

probinson added inline comments.Feb 16 2018, 11:51 AM
test/DebugInfo/Generic/inlined-strings.ll
38 ↗(On Diff #134625)

Actually I meant the above 3 CHECK lines were all you needed, for the entire test. Sorry for not making that clear.

ABataev added inline comments.Feb 16 2018, 11:58 AM
test/DebugInfo/Generic/inlined-strings.ll
38 ↗(On Diff #134625)

But these new checks are still good?

probinson added inline comments.Feb 16 2018, 12:35 PM
test/DebugInfo/Generic/inlined-strings.ll
38 ↗(On Diff #134625)

They are overkill. The 3 lines from my previous comment are all you need.

Instead of all the elaborate checks, you could use llvm-dwarfdump -debug-str (dump just the .debug_str section) and then use CHECK-NOT: " to prove it's empty.

test/DebugInfo/Generic/inlined-strings.ll
4 ↗(On Diff #134667)

You can avoid generating the temp file here. Instead of > %t you do -o - and then pipe it to llvm-dwarfdump with an input file of -.

Instead of all the elaborate checks, you could use llvm-dwarfdump -debug-str (dump just the .debug_str section) and then use CHECK-NOT: " to prove it's empty.

Actually never mind. Losing all the debug info would also pass that check, so my previous 3-line recommendation stands.

ABataev updated this revision to Diff 134711.Feb 16 2018, 1:31 PM

Fixed test checks.

probinson accepted this revision.Feb 16 2018, 1:40 PM

Fix the temp-file thing in the test and LGTM.

This revision is now accepted and ready to land.Feb 16 2018, 1:40 PM
echristo accepted this revision.Feb 16 2018, 2:07 PM

One inline typo, otherwise fix up what Paul asked for and LGTM.

-eric

lib/CodeGen/AsmPrinter/DwarfDebug.h
497 ↗(On Diff #134711)

wether - > whether

This revision was automatically updated to reflect the committed changes.

What's the motivation for this change? Do you have a platform that doesn't support debug_string?

What's the motivation for this change? Do you have a platform that doesn't support debug_string?

Yep. Exactly. Apparently nvptx doesn't.