This is an archive of the discontinued LLVM Phabricator instance.

[test][llvm-dwarfdump] Add extra test case for invalid MD5 form
ClosedPublic

Authored by jhenderson on Jan 3 2020, 7:39 AM.

Details

Summary

A subsequent patch will change how an invalid file name table is handled to allow parsing to continue. This patch adds a test case that will demonstrate a difference in behaviour with that change between invalid file tables where the error is before the end of the stated prologue length and where the error occurs after the stated length.

Diff Detail

Event Timeline

jhenderson created this revision.Jan 3 2020, 7:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 3 2020, 7:39 AM

Early ping - I'd like to get this and the other related reviews in before the release branch is created if possible.

I guess these error messages could be more precise about what's invalid & why it's invalid. In this case I guess it's invalid because the DW_LNCT_MD5 has a form that contains only one byte, not large enough to encode a whole MD5 sum?

& this is to demonstrate that other things can still be parsed in spite of that failure?

I guess these error messages could be more precise about what's invalid & why it's invalid. In this case I guess it's invalid because the DW_LNCT_MD5 has a form that contains only one byte, not large enough to encode a whole MD5 sum?

I agree with making the error message more precise, but that's probably a different change. It's invalid as you say due to the bad form (I think that's the only way it can be invalid).

& this is to demonstrate that other things can still be parsed in spite of that failure?

Right. It goes with the follow-on change D72158 that shows that parsing will continue after the failure. Following that change, there'll be two different cases to handle, which result in different sets of warnings. Note that in D72158 one of the two MD5 invalid hashes results in a "size doesn't match" warning, in addition to the current ones and the other doesn't. We need to have the different cases to show that the offset is updated appropriately after the failure.

dblaikie accepted this revision.Jan 17 2020, 2:58 PM

Looks good - James, I might've asked this on another thread - but could you write up a brief goals/design email of what your overall goal is with these patches? There seem to be enough of them that it'd be helpful to link to in each review (and summarize where the patch fits in the broader scheme of things) & provide framing when reviewing patches to better understand the overall goals.

This revision is now accepted and ready to land.Jan 17 2020, 2:58 PM

Looks good - James, I might've asked this on another thread - but could you write up a brief goals/design email of what your overall goal is with these patches? There seem to be enough of them that it'd be helpful to link to in each review (and summarize where the patch fits in the broader scheme of things) & provide framing when reviewing patches to better understand the overall goals.

Thanks. As you probably saw, I did previously write up comments in D72155. I'll post something on the mailing list at some point in the next couple of days, in order to clarify everything.

This revision was automatically updated to reflect the committed changes.