This is an archive of the discontinued LLVM Phabricator instance.

[DWARFv5] Tolerate files not all having an MD5 checksum.
ClosedPublic

Authored by probinson on Jun 13 2018, 9:53 AM.

Details

Summary

In some cases, for example when compiling a preprocessed file, the
front-end is not able to provide an MD5 checksum for all files. When
that happens, omit the MD5 checksums from the final DWARF, because
DWARF doesn't have a way to indicate that some but not all files have
a checksum.

When assembling a .s file, and some but not all .file directives
provide an MD5 checksum, issue a warning and don't emit MD5 into the
DWARF.

Fixes PR37623.

Diff Detail

Repository
rL LLVM

Event Timeline

probinson created this revision.Jun 13 2018, 9:53 AM
aprantl added inline comments.Jun 13 2018, 10:13 AM
llvm/include/llvm/MC/MCDwarf.h
258 ↗(On Diff #151189)

I found that a little confusing to read at first:
what do you think about

Header.HasAnyMD5 |= CheckSum;
Header.HasAllMD5 &= CheckSum;

to underline the fact that these values are updated incrementally?

probinson marked an inline comment as done.
probinson removed a reviewer: espindola.

Simplify bool updates.

probinson added inline comments.Jun 13 2018, 11:11 AM
llvm/include/llvm/MC/MCDwarf.h
258 ↗(On Diff #151189)

It would have to be spelled bool(CheckSum) or maybe you would prefer (CheckSum != nullptr)) but yes that does work.

JDevlieghere added inline comments.Jun 13 2018, 11:27 AM
llvm/include/llvm/MC/MCDwarf.h
255 ↗(On Diff #151206)

How do you feel about a convenience method that takes the header and checksum pointer and updates both?

Privatize the flags for tracking MD5 usage.

probinson marked an inline comment as done.Jun 13 2018, 12:26 PM
probinson added inline comments.
llvm/include/llvm/MC/MCDwarf.h
255 ↗(On Diff #151206)

What, because the same code is in 4 places and I already made mistakes updating all 4 consistently, I should factor them into a common method? You're right!
After which it really made sense to make them private fields and bury all of it behind an API for proper data hiding.

JDevlieghere accepted this revision.Jun 13 2018, 1:09 PM

LGTM!

llvm/include/llvm/MC/MCDwarf.h
255 ↗(On Diff #151206)

👍👍👍

This revision is now accepted and ready to land.Jun 13 2018, 1:09 PM
This revision was automatically updated to reflect the committed changes.
probinson marked an inline comment as done.