Page MenuHomePhabricator

[DebugInfo] Reject line tables of version > 5
ClosedPublic

Authored by jhenderson on Fri, Feb 7, 2:33 AM.

Details

Summary

If a debug line section with version of greater than 5 is encountered, prior to this change the parser would accept it and treat it as version 5. This might work to some extent, but then it might not at all, as it really depends on the format of the unspecified future version, which will be different (otherwise there would be no point in changing the version number). Any information we could provide has a good chance of being invalid, so we should just refuse to parse such tables.

Depends on D74202.

Diff Detail

Event Timeline

jhenderson created this revision.Fri, Feb 7, 2:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Feb 7, 2:33 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
MaskRay accepted this revision.Fri, Feb 7, 11:20 AM
MaskRay added inline comments.
llvm/test/tools/llvm-dwarfdump/X86/debug_line_invalid.test
67 ↗(On Diff #243112)

max -> the max?

This revision is now accepted and ready to land.Fri, Feb 7, 11:20 AM
dblaikie accepted this revision.Fri, Feb 7, 11:22 AM

Might be worth a different strategy (either add to the end, and/or use stable names rather than numbers). But otherwise seems fine.

Other thoughts: Probably print the unknown version number in decimal rather than hex? Probably doesn't need both an assembly+dumper test and a unit test. One or the other (which ever is most convenient - the unit test looks simpler/more self contained & targeted) is fine by me - assuming the general codepath for error handling is already tested for the dumper, for instance. (ie: the changed code is exercised in the new test, doesn't mean all paths that reach that changed code need to be retested).

llvm/test/tools/llvm-dwarfdump/X86/debug_line_invalid.test
158 ↗(On Diff #243112)

Perhaps numbering cases isn't a good idea if new cases are going to be added in the middle & shift all the numbers? Perhaps a short textual name would be better, then? (like test cases in a GUnit test, for instance)

Might be worth a different strategy (either add to the end, and/or use stable names rather than numbers). But otherwise seems fine.

I think I'm going to do a follow-up review to remove the numbers. Previously, all my new cases were added to the end, but this test case is strongly related to the other version ones, so I thought it should belong with it.

Other thoughts: Probably print the unknown version number in decimal rather than hex?

Yeah, printing in decimal makes sense. I'd like to do that in a follow-up, as it's a change to a pre-existing message.

Probably doesn't need both an assembly+dumper test and a unit test. One or the other (which ever is most convenient - the unit test looks simpler/more self contained & targeted) is fine by me - assuming the general codepath for error handling is already tested for the dumper, for instance. (ie: the changed code is exercised in the new test, doesn't mean all paths that reach that changed code need to be retested).

Seems okay to me (I'd be inclined to keep the unit test here). I've been uncertain where to draw the distinction between the two, if I'm honest, and a number of my other recent changes have added both. On a previous code base I worked on, it was generally expected that there were both unit tests and system (i.e. lit-equivalent) testing for any change, so I've tended to err on the side of over-testing rather than under-testing.

This revision was automatically updated to reflect the committed changes.

Might be worth a different strategy (either add to the end, and/or use stable names rather than numbers). But otherwise seems fine.

I think I'm going to do a follow-up review to remove the numbers. Previously, all my new cases were added to the end, but this test case is strongly related to the other version ones, so I thought it should belong with it.

Sounds good to me.

Other thoughts: Probably print the unknown version number in decimal rather than hex?

Yeah, printing in decimal makes sense. I'd like to do that in a follow-up, as it's a change to a pre-existing message.

Sure sure.

Probably doesn't need both an assembly+dumper test and a unit test. One or the other (which ever is most convenient - the unit test looks simpler/more self contained & targeted) is fine by me - assuming the general codepath for error handling is already tested for the dumper, for instance. (ie: the changed code is exercised in the new test, doesn't mean all paths that reach that changed code need to be retested).

Seems okay to me (I'd be inclined to keep the unit test here). I've been uncertain where to draw the distinction between the two, if I'm honest, and a number of my other recent changes have added both. On a previous code base I worked on, it was generally expected that there were both unit tests and system (i.e. lit-equivalent) testing for any change, so I've tended to err on the side of over-testing rather than under-testing.

I'll admit I'm a bit fussy about testing - both too much and too little can be maintenance problems & LLVM's choices aren't necessarily the best, nor applicable to all other projects/types of code.

My basic rule of thumb is: "If it's unlikly/impossible to introduce a bug that will cause only this test to fail and no others, then this test is probably unnecessary/excessive". And while LLVM historically tends towards end-to-end testing (where the end-to-end might be quite small, using some targeted tool like llvm-dwarfdump, opt, etc) with relatively little unit testing - the latter can be the right tool for the job in some situations & this may be one of them. Where the end-to-end error handling path is tested, but a smaller/easier to read/etc targeted unit test can then exercise the various sources of errors, without having to test all the way through the end-to-end every for every one.