This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Improvements to representation of enumeration types (PR36168)
ClosedPublic

Authored by chill on Jan 31 2018, 6:30 AM.

Details

Summary

This patch is the LLVM part of fixing the issues, described in https://bugs.llvm.org/show_bug.cgi?id=36168

  • The representation of enumerator values in the debug info metadata now contains a boolean flag isUnsigned, which determines how the bits of the value are interpreted.
  • The DW_TAG_enumeration type DIE now always (for DWARF version >= 3) includes a DW_AT_type attribute, which refers to the underlying integer type, as suggested in DWARFv4 (5.7 Enumeration Type Entries).
  • The debug info metadata for enumeration type contains (in flags) indication whether this is a C++11 "fixed enum".
  • For C++11 enumeration with a fixed underlying type, the DIE also includes the DW_AT_enum_class attribute (for DWARF version >= 4).
  • Encoding of enumerator constants uses DW_FORM_sdata for signed values and DW_FORM_udata for unsigned values, as suggested by DWARFv4 (7.5.4 Attribute Encodings).

The changes should be backwards compatible:

  • the isUnsigned attribute is optional and defaults to false.
  • if the underlying type for the enumeration is not available, the enumerator values are considered signed.
  • the FixedEnum flag defaults to clear.
  • the bitcode format for DIEnumerator stores the unsigned flag bit #1 of the first record element, so the format does not change and the zero previously stored there is consistent with the false default for IsUnsigned.

There are corresponding changes to clang (I'll submit them shortly), however this patch in isolation
ought to not break build or introduce regressions.

Diff Detail

Repository
rL LLVM

Event Timeline

chill created this revision.Jan 31 2018, 6:30 AM

Thanks!

From what I can tell this is missing a bitcode upgrade in MetadataLoader to parse the old format and upgrade it (e.g, by treating everything as signed, similar to how our current implementation works).
Can you add that and a bitcode upgrade test to test/Bitcode that exercises it?

Do you need to add any additional checks to the IR Verifier?

aprantl requested changes to this revision.Jan 31 2018, 9:29 AM
This revision now requires changes to proceed.Jan 31 2018, 9:29 AM
aprantl added inline comments.Jan 31 2018, 9:34 AM
include/llvm/IR/DebugInfoMetadata.h
399 ↗(On Diff #132150)

I think you can squeeze this bit into the classdata field as some other DI* types do.

dblaikie added inline comments.Jan 31 2018, 10:56 AM
test/DebugInfo/Generic/debug-info-enum.ll
29–39 ↗(On Diff #132150)

Usually we don't mix the CHECK lines in with the IR - though I wouldn't rule it out.

Also usually we provide the C or C++ source (or something like it) in a comment a the top to see at a glance what's being tested)

I actually don't mind this test as-is, but just some ideas.

test/DebugInfo/X86/enum-class.ll
47 ↗(On Diff #132150)

Probably just drop the abbreviation number from the test

chill updated this revision to Diff 132225.Jan 31 2018, 11:19 AM

Update:

  • accept also pre-patch Bitcode form of DIEnumerator's
  • add an upgrade test
chill marked an inline comment as done.Jan 31 2018, 11:21 AM
aprantl added inline comments.Jan 31 2018, 11:25 AM
lib/Bitcode/Reader/MetadataLoader.cpp
1200 ↗(On Diff #132225)

An alternative implementation would be to stick the isSigned bit into Record[0] & 0x2. This would make the on-disk-representation smaller. There are lots of examples of other entities in the DIType hierarchy that do this.

chill updated this revision to Diff 132415.Feb 1 2018, 9:58 AM
chill edited the summary of this revision. (Show Details)

Changes relative to the previous version:

  • Signed/unsigned flag stored inside SubclassData32 member of DIEnumerator.
  • Metadata for DIEnumerator is back to 3-element record. The signed/unsigned flag is stored in bit #1 of the first record element. Since it looked a bit weird to store IsSigned value of true as 0, ...
  • ... changed everywhere IsSigned to IsUnsigned
  • removed the upgrade test, as the bitcode format doesn't actually change now
chill marked 2 inline comments as done.Feb 1 2018, 9:59 AM

Did you perhaps forget to add the upgrade test before uploading?

There should be a bitcode upgrade test in test/Bitcode with a .bc file that contains an old-style enum, and a round-trip test in test/IR that tests the new enums.

Otherwise I think this looks good, thanks!

  • removed the upgrade test, as the bitcode format doesn't actually change now

Oh I see. Yes, a round-trip test (llvm-as | llvm-dis | llvm-as | llvm-dis) in lib/IR would be enough now.

chill updated this revision to Diff 132566.Feb 2 2018, 3:47 AM

Changes relative to the prevuious revision:

  • added an LLVM IR <-> Bitcode round-trip test for enums
aprantl accepted this revision.Feb 2 2018, 8:45 AM

Looks good with one outstanding change applied. Please wait for @dblaikie to sign off, too, before landing this.

test/Bitcode/enum-round-trip.ll
1 ↗(On Diff #132566)

Please rename this file to test/Assembler/DIEnumerator.ll

This revision is now accepted and ready to land.Feb 2 2018, 8:45 AM
chill updated this revision to Diff 132609.Feb 2 2018, 8:54 AM
chill marked an inline comment as done.
dblaikie accepted this revision.Feb 2 2018, 5:12 PM

Looks good - thanks!

lib/AsmParser/LLParser.cpp
3939–3943 ↗(On Diff #132609)

Could invert this (& similarly below) to reduce indentation & bracing:

if (ParseMDField(...))
  return true;
Result.assign(Res);
return false;
test/Assembler/DIEnumerator.ll
24–64 ↗(On Diff #132609)

Probably add some whitespace in here at the beginning of each enum so it's easier to read/visually separate each test case?

test/DebugInfo/Generic/debug-info-enum.ll
24 ↗(On Diff #132609)

Worth a comment at the start of each enumeration/test case describing what this test covers that the others don't? So it's clear what to focus on? (just a short, english language (rather than metadata/check lines) summary of the important features "test a fixed enum class over signed char" or the like)

chill updated this revision to Diff 132824.Feb 5 2018, 6:55 AM
chill marked 3 inline comments as done.
This revision was automatically updated to reflect the committed changes.

Hi, Momchil. I suspect this change caused

Assertion failed: (DTy), function constructEnumTypeDIE, file /Users/buildslave/jenkins/workspace/clang-stage1-configure-RA/llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp, line 1436.

on the build bot http://green.lab.llvm.org/green/job/clang-stage2-Rthinlto/ For example, see http://green.lab.llvm.org/green/job/clang-stage2-Rthinlto/8694/console

Earlier instead of assert(DTy); we had

if (DTy) {
  addType(Buffer, DTy);
  addFlag(Buffer, dwarf::DW_AT_enum_class);
}

I don't know where the bug is but maybe it makes sense to remove assertion for now and correctly handle nullptr DTy? This will unblock the build bot for others and after that you can work on better fix if required.

llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
1435

Talking about this assertion.

chill added a comment.Feb 14 2018, 2:42 AM

Hi, Momchil. I suspect this change caused

Assertion failed: (DTy), function constructEnumTypeDIE, file /Users/buildslave/jenkins/workspace/clang-stage1-configure-RA/llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp, line 1436.

on the build bot http://green.lab.llvm.org/green/job/clang-stage2-Rthinlto/ For example, see http://green.lab.llvm.org/green/job/clang-stage2-Rthinlto/8694/console

Earlier instead of assert(DTy); we had

if (DTy) {
  addType(Buffer, DTy);
  addFlag(Buffer, dwarf::DW_AT_enum_class);
}

I don't know where the bug is but maybe it makes sense to remove assertion for now and correctly handle nullptr DTy? This will unblock the build bot for others and after that you can work on better fix if required.

Hello,

This is quite strange. Having the flag DINode::FlagFixedEnum set means the LLVM IR was generated by this
https://github.com/llvm-mirror/clang/blob/7b1bfe45423fcf2ac09f3b5cb7a7ef20596254ed/lib/CodeGen/CGDebugInfo.cpp#L2513
version of Clang (as the last parameter to createEnumerationType did not exist before and defaults to false).

Perhaps the underlying type was null already in the AST?

For pre-existing .ll files, the flag DINode::FlagFixedEnum should be clear, and the control shouldn't even enter that if body.

Hi, Momchil. I suspect this change caused

Assertion failed: (DTy), function constructEnumTypeDIE, file /Users/buildslave/jenkins/workspace/clang-stage1-configure-RA/llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp, line 1436.

on the build bot http://green.lab.llvm.org/green/job/clang-stage2-Rthinlto/ For example, see http://green.lab.llvm.org/green/job/clang-stage2-Rthinlto/8694/console

Earlier instead of assert(DTy); we had

if (DTy) {
  addType(Buffer, DTy);
  addFlag(Buffer, dwarf::DW_AT_enum_class);
}

I don't know where the bug is but maybe it makes sense to remove assertion for now and correctly handle nullptr DTy? This will unblock the build bot for others and after that you can work on better fix if required.

Hello,

This is quite strange. Having the flag DINode::FlagFixedEnum set means the LLVM IR was generated by this
https://github.com/llvm-mirror/clang/blob/7b1bfe45423fcf2ac09f3b5cb7a7ef20596254ed/lib/CodeGen/CGDebugInfo.cpp#L2513
version of Clang (as the last parameter to createEnumerationType did not exist before and defaults to false).

Perhaps the underlying type was null already in the AST?

For pre-existing .ll files, the flag DINode::FlagFixedEnum should be clear, and the control shouldn't even enter that if body.

So, do you plan to fix the regression? And if yes, how much time do you need? If the fix isn't expected soon, I'll revert the change.