This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Unify ChecksumKind and Checksum value in DIFile
ClosedPublic

Authored by scott.linder on Feb 7 2018, 1:40 PM.

Details

Summary

Rather than encode the absence of a checksum with a Kind variant, instead put both the kind and value in a struct and wrap it in an Optional.

Diff Detail

Event Timeline

scott.linder created this revision.Feb 7 2018, 1:40 PM
aprantl added inline comments.Feb 7 2018, 3:59 PM
include/llvm/IR/DebugInfoMetadata.h
501–502

These should be /// Doxygen comments.

521

If this is a string, why does this need to be a template?

lib/Bitcode/Reader/MetadataLoader.cpp
1357

When changing Bitcode, please always add a .bc upgrade test of the old format to test/Bitcode

JDevlieghere added inline comments.Feb 8 2018, 1:50 AM
include/llvm/IR/DebugInfoMetadata.h
506

Why did you not choose to make this part of the ChecksumInfo struct?

521

It's used for both MDStrings and StringRefs.

587–588

Would it make sense to move these into ChecksumInfo too?

lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
177

Why do we have/need two enums to express the same thing? Are the values different?

Added Doxygen comment for DIFile::ChecksumKind.

scott.linder marked 3 inline comments as done.Feb 8 2018, 7:53 AM
scott.linder added inline comments.
include/llvm/IR/DebugInfoMetadata.h
506

They do not depend on the template parameter, so this saves having two instances of ChecksumKind in the typesystem, namely ChecksumInfo<MDString *>::ChecksumKind and ChecksumInfo<StringRef>::ChecksumKind.

587–588

Similar to above, these are static methods which do not depend on the templated string type (and never will), so I chose to leave them outside. Moving them inside just creates multiple, equivalent names for them.

Add Bitcode test generated with LLVM 5.0.1 to check backwards compatibility.

scott.linder marked an inline comment as done.Feb 8 2018, 9:15 AM
scott.linder added inline comments.
lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
177

The CodeView enum was already defined, but the previous code simply cast the DIFile::ChecksumKind to unsigned here, as the values happen to line up. This is somewhat fragile, and will only work for one format (why align the DIFile encoding with the CodeView encoding? why not DWARF?). I think being explicit is better here, and so when I had to make a change anyway I added the switch.

JDevlieghere accepted this revision.Feb 9 2018, 3:40 AM

Thanks Scott! This LGTM, but let's wait for @aprantl to have another look before merging this.

include/llvm/IR/DebugInfoMetadata.h
506

Makes sense, thanks!

This revision is now accepted and ready to land.Feb 9 2018, 3:40 AM
aprantl added inline comments.Feb 9 2018, 8:40 AM
lib/Bitcode/Writer/BitcodeWriter.cpp
1555

The comment in MetadataLoader claims that the None kind uses 0 as a representation. Why is this condition necessary?

test/Bitcode/upgrade-dbg-checksum.ll
16

I don't think you need a function or a global at all. An empty module with just two DICompileUnits should be sufficient.

scott.linder added inline comments.Feb 9 2018, 11:50 AM
lib/Bitcode/Writer/BitcodeWriter.cpp
1555

I'm not sure I understand what you are asking; the old code would have implicitly done the same here, as CSK_None was encoded as 0. In the new code the None variant of the Optional does not have an "encoding", so we have to explicitly encode each part of the struct, even if the Optional is None. In order to retain backwards compatibility, we still reserve null bytes for this purpose, and I documented this in comments on each end and in the ChecksumKind enum.

test/Bitcode/upgrade-dbg-checksum.ll
16

You are right, I will minimize the test case.

Minimized the Bitcode test case.

aprantl accepted this revision.Feb 9 2018, 1:31 PM
This revision was automatically updated to reflect the committed changes.