This is an archive of the discontinued LLVM Phabricator instance.

[llvm][DebugInfo] Backport DW_AT_default_value for template args
ClosedPublic

Authored by Michael137 on Dec 13 2022, 9:56 AM.

Details

Summary

Summary

Starting with DWARFv5, DW_AT_default_value can be used to indicate
that a template argument has a default value. With this patch LLVM
will emit the this attribute earlier versions of DWARF, unless
compiling with -gstrict-dwarf.

Diff Detail

Event Timeline

Michael137 created this revision.Dec 13 2022, 9:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 13 2022, 9:56 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
Michael137 requested review of this revision.Dec 13 2022, 9:56 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptDec 13 2022, 9:56 AM
dblaikie accepted this revision.Dec 13 2022, 12:31 PM

Looks good to me - maybe minor optional tweaks to the test.

clang/test/CodeGenCXX/debug-info-template-parameter.cpp
37

Could consider using DWARF-DUMP-LABEL for the TAGs here (& maybe an extra DWARF-DUMP-LABEL: DW_TAG at the end) so that the attributes are constrained to be within the same tag? (could use DWARF-DUMP-DAG to test attributes in a way that's not order-specific, which would be nice - but I don't know that we do that much in the dwarfdump-driven tests, so not a huge deal)

41

NOT checks should be pretty general (so they don't accidentally pass despite minor differences-from-expectation) so maybe this should STRICT-NOT: DW_AT_default_value?

This revision is now accepted and ready to land.Dec 13 2022, 12:31 PM
Michael137 added inline comments.Dec 13 2022, 4:30 PM
clang/test/CodeGenCXX/debug-info-template-parameter.cpp
37

+1 for the DAG directive

Could you elaborate on the -LABEL though? IIUC the labels have to match a line in the file uniquely, which the DW_TAGs wouldn't

41

Makes sense!

Michael137 added inline comments.Dec 13 2022, 4:34 PM
clang/test/CodeGenCXX/debug-info-template-parameter.cpp
37

Could label the DW_AT_name perhaps? Unless I'm misunderstanding

  • Use CHECK-DAG for attributes
  • Also compile with non-strict DWARFv4
  • Widen the STRICT-NOT checks
Michael137 marked an inline comment as done.Dec 13 2022, 5:03 PM

IIUC the labels have to match a line in the file uniquely, which the DW_TAGs wouldn't

CHECK-LABEL doesn't have to match a line in the file uniquely. What happens is that all the -LABEL directives are processed first, in order, subdividing the input text into regions. Then the non-LABEL directives are processed within their respective regions.

So you could have

CHECK-LABEL: DW_TAG
CHECK: DW_AT_name ("foo")
CHECK-LABEL: DW_TAG
CHECK: DW_AT_location
CHECK-LABEL: DW_TAG

which would search only the first tag for "foo" and only the second tag for the location attribute.

CHECK-LABEL doesn't have to match a line in the file uniquely.

I mean, it's good practice if they do match uniquely; that way you don't get excessively confusing results when the output changes, and things start matching where you didn't expect. But it's not a requirement.

dblaikie accepted this revision.Dec 14 2022, 12:07 PM

I think this is good - with some check after the template list to constrain the dag/not checks to only be within the intended tag, not accidentally skip past into some other tag.

clang/test/CodeGenCXX/debug-info-template-parameter.cpp
26

typo? (DUMPABEL)

37

Oh, yeah, unless the tests small enough to have unique/simple DW_TAGs you could -LABEL, this is probably all the reason why we don't DAG in dwarfdump tests usually.

41

Probably good to end this list of DAG/NOT with a DWARF-DUMP: {{DW_TAG|NULL}} to ensure these attribute matches don't happen to end up matching some other tag after the template parameters?

  • Fix label name
  • Add check for end of block
Michael137 marked an inline comment as done.Dec 14 2022, 2:31 PM
This revision was landed with ongoing or failed builds.Dec 14 2022, 2:32 PM
This revision was automatically updated to reflect the committed changes.
chapuni added inline comments.
clang/test/CodeGenCXX/debug-info-template-parameter.cpp
7

llvm-dwarfdump fails if x86 is unconfigured.

Michael137 added inline comments.Dec 14 2022, 5:30 PM
clang/test/CodeGenCXX/debug-info-template-parameter.cpp
7

I suppose we could just stop passing the triple explicitly

aprantl added inline comments.Dec 14 2022, 5:34 PM
clang/test/CodeGenCXX/debug-info-template-parameter.cpp
7

I think it would be better to check for the LLVM IR here, and add a second test that compiles that LLVM IR with llc in llvm/test/DebugInfo/X86

aprantl added inline comments.Dec 14 2022, 5:36 PM
clang/test/CodeGenCXX/debug-info-template-parameter.cpp
7

Generally, no clang fronted tests should depend on any target to be configured.

Michael137 added inline comments.Dec 14 2022, 6:49 PM
clang/test/CodeGenCXX/debug-info-template-parameter.cpp
7