Page MenuHomePhabricator

Permit multiple .file directives with -g
AbandonedPublic

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.

cmtice added a subscriber: cmtice.Jun 5 2019, 4:03 PM

Ping? Is anybody looking at or working on this? The bug this fixes is one of the issues currently blocking Chrome OS from being able to adopt Dwarf v5, so we are hoping for a fix?

Herald added a project: Restricted Project. · View Herald TranscriptJun 5 2019, 4:03 PM
bcain added a comment.Jun 5 2019, 4:32 PM

Ping? Is anybody looking at or working on this? The bug this fixes is one of the issues currently blocking Chrome OS from being able to adopt Dwarf v5, so we are hoping for a fix?

Hmm, apologies. I will take a look tomorrow to try and pick this back up. But unfortunately I'll be away on vacation after tomorrow and it may not be addressed soon. Is Dwarfv5 for ChromeOS bound to a particular llvm release or release date?

cmtice added a comment.Jun 5 2019, 4:44 PM

Ping? Is anybody looking at or working on this? The bug this fixes is one of the issues currently blocking Chrome OS from being able to adopt Dwarf v5, so we are hoping for a fix?

Hmm, apologies. I will take a look tomorrow to try and pick this back up. But unfortunately I'll be away on vacation after tomorrow and it may not be addressed soon. Is Dwarfv5 for ChromeOS bound to a particular llvm release or release date?

No particular release or release date at the moment; just trying to make it work "as soon as we can". :-)

jmorse added a subscriber: jmorse.Jun 6 2019, 2:26 AM

As I mentioned in PR38449, I plan to look at this starting this week. Even benign situations such as

foo:
.file 1 "a.c"

are tripping over the assertion. I think the correct tactic is not to remove the places that are doing the checks, but make those places do a better job of tidying up anything that had been done in response to the command-line -g in favor of allowing the embedded directives to DTRT.

bcain abandoned this revision.Thu, Jun 27, 8:04 AM

Obsoleted by r364039.