This is an archive of the discontinued LLVM Phabricator instance.

[Debug-Info] Use inlined strings in .dwinfo section by default for DBX.
ClosedPublic

Authored by Esme on Apr 6 2021, 2:56 AM.

Details

Summary

This is a minor patch to set the default DwarfInlinedStrings as inlined strings for DBX, due to DBX does not support .dwstr section for now.

Diff Detail

Event Timeline

Esme created this revision.Apr 6 2021, 2:56 AM
Esme requested review of this revision.Apr 6 2021, 2:56 AM
Herald added a project: Restricted Project. · View Herald Transcript

Can we set this only for DBX? This should be a limitation for DBX not for AIX?

Esme updated this revision to Diff 335544.Apr 6 2021, 9:11 AM
Esme retitled this revision from [Debug-Info] Use inlined strings in .dwinfo section by default on AIX. to [Debug-Info] Use inlined strings in .dwinfo section by default for DBX..
Esme edited the summary of this revision. (Show Details)
Esme added inline comments.Apr 6 2021, 9:24 AM
llvm/lib/CodeGen/CommandFlags.cpp
417 ↗(On Diff #335544)

Thanks for @shchenz's comments.
We have added tuning debugger option DBX in D99400, but missed adding it to llc/opt command flags, which is a one-line change, so I also do it in this patch as well. Is it ok?

dblaikie added inline comments.Apr 6 2021, 10:10 AM
llvm/test/DebugInfo/XCOFF/dbx-inlinedstrings.ll
32–33 ↗(On Diff #335544)

I'd probably put these CHECK lines up the top near the RUN line - and flesh them out a bit, maybe? Oh, and in the RUN line, run llc to an object, run the object through llvm-dwarfdump rather than checking the assembly directly. Possibly use implicit-check-not DW_FORM_strp, and explicitly check where the DW_FORM_string turns up (perhaps dump debug_info in verbose mode, and check, for instance that the DW_AT_producer is in the right form and has the right value)

(do you have a pointer to the testing that was originally added for this feature when it was introduced for NVPTX? Might be worth comparing it/seeing if there's some inspiration there)

shchenz added inline comments.Apr 6 2021, 10:20 PM
llvm/lib/CodeGen/CommandFlags.cpp
417 ↗(On Diff #335544)

The change here is for controlling the inlined string for llc. It is not necessary. Normally, when you compile source files by using clang, clang FE will set the debugger type for you. You should already test this, right?

Wth this change, now you can test the inlined string settings only with llc.

Esme updated this revision to Diff 335777.Apr 7 2021, 4:27 AM

Addressed comments.

llvm/lib/CodeGen/CommandFlags.cpp
417 ↗(On Diff #335544)

I've verified both of clang -gdbx under Linux and clang -g under AIX, and clang FE works well for them.

llvm/test/DebugInfo/XCOFF/dbx-inlinedstrings.ll
32–33 ↗(On Diff #335544)

The feature for NVPTX introduced in D41827 is not the main purpose in that patch, so the whole assembly was checked directly there.

dblaikie added inline comments.Apr 7 2021, 12:27 PM
llvm/test/DebugInfo/XCOFF/dbx-inlinedstrings.ll
32–33 ↗(On Diff #335544)

fair enough - thanks for the test update, it's probably a bit overkill here. Testing just the DW_AT_producer, for instance, seems like it'd be good enough to demonstrate the use of FORM_string and the value. No need to test a bunch of other (especially non-string) attributes (that sort of testing can make the test brittle - if we add new attributes, reorder attributes, etc, this test would need to be updated despite the functionality it intends to test not having changed)

Esme updated this revision to Diff 335974.EditedApr 7 2021, 7:11 PM

Removed unnecessary checks.

@dblaikie Thanks! Your advice makes sense to me.

dblaikie accepted this revision.Apr 7 2021, 7:51 PM

Looks good - thanks!

This revision is now accepted and ready to land.Apr 7 2021, 7:51 PM
This revision was landed with ongoing or failed builds.Apr 8 2021, 12:21 AM
This revision was automatically updated to reflect the committed changes.