Page MenuHomePhabricator

Permit multiple .file directives with -g
Needs ReviewPublic

Authored by bcain on Nov 9 2018, 1:07 PM.

Details

Reviewers
probinson
dyung
Summary

PR38994 caught a defect introduced by 340839: multiple .file directives
along with -g would result in an assertion. This change no longer resets
the implicit '.file 0' and doesn't disable the generation of debug info.

Diff Detail

Event Timeline

bcain created this revision.Nov 9 2018, 1:07 PM
bcain added a comment.Nov 9 2018, 1:27 PM

The directive_file-4.s test passes and no existing tests fail with this change.

Paul, you suggested keeping a table of the source locs for labels that gets flushed at ".file 1 foo". This doesn't implement that, but it appears to resolve the assertion in the provided test case. Do you think it's sufficient?

Also, we lose the power to detect "file number already allocated" with this change -- I deleted it to avoid failures introduced by the other change. So I suspect this patch may not be quite right as is, but I'd happily take suggestions on a better approach.

llvm/test/MC/AsmParser/directive_file-5.s
7–12

TBD: nothing has yet been added to reject this code.

dblaikie added a subscriber: dblaikie.

Looks like the original functionality was added here in r109651 without any test case... so it's hard to say that because your change causes no regression that it's correct, because there's a lack of test coverage (no test tests for this message about "file number already allocated").

So probably need to have some discussion about when/where/if this should fire.

bcain added a comment.Nov 12 2018, 3:32 PM

...

So probably need to have some discussion about when/where/if this should fire.

I'm less concerned about "file number already allocated" than I am about the approach in general. Since I bypassed Paul's suggestion, does this design really represent the right approach? Are the expected results in "CHECK-DEBUG/CHECK-DEBUG-ASM" correct? Should the test case plumb deeper to look for how the label's src location is stored?

ormris added a subscriber: ormris.Nov 21 2018, 10:21 AM

It looks like this patch would fail to reject cases such as:

.file 1 "a.c"
.file 1 "b.c"

and if that's true, it's a problem.