Page MenuHomePhabricator

[MC][DWARF] Corrected handling of is_stmt flag in .loc directives
ClosedPublic

Authored by dp on Apr 14 2020, 5:46 AM.

Details

Summary

According to DWARF standard, is_stmt is a global flag; when set or cleared it should affect subsequent .loc directives.

However llvm assembler handled is_stmt differently: it forced all locations to have is_stmt=1 unless is_stmt was specified explicitly as 0.

The proposed fix utilizes current DWARF state flags to compute correct is_stmt values.

See bug 45529 for a detailed issue description.

Diff Detail

Event Timeline

dp created this revision.Apr 14 2020, 5:46 AM
jmorse added a subscriber: jmorse.Apr 14 2020, 5:57 AM

Should include the more detailed description in the actual commit message before submit

llvm/test/MC/AsmParser/directive_loc_2.s
2

I would expect this needs to be in an x86 subdirectory or something, but it looks like none of the other tests here bother to check if x86 is built?

dblaikie added a subscriber: dblaikie.
dblaikie added subscribers: aprantl, JDevlieghere, probinson.
dp marked an inline comment as done.Apr 14 2020, 9:33 AM
dp added inline comments.
llvm/test/MC/AsmParser/directive_loc_2.s
2

Looks like x86 is the default target for testing generic functionality.

aprantl added inline comments.Apr 15 2020, 4:04 PM
llvm/test/MC/AsmParser/directive_loc_2.s
2

So what happens if you build llvm without an X86 target and run check-llvm-mc?

dp marked an inline comment as done.Apr 16 2020, 4:21 AM
dp added inline comments.
llvm/test/MC/AsmParser/directive_loc_2.s
2

The tests will be skipped as unsupported - see lit.local.cfg.

As mentioned above, the commit log should have a short standalone description of the problem/fix. It's fine to cite the bug report as well, but someone doing a "git log" should be able to tell what your patch is about without having to click links or look elsewhere.

llvm/test/MC/AsmParser/directive_loc_2.s
19

Don't bother dumping with -debug-info or checking for an empty .debug_info section; it distracts from the purpose of this test.

25

To check for the absence of is_stmt you could put {{$}} at the end of this line; then you don't need the separate CHECK-NOT.
Same comment for the CHECK looking at line 6, below.

dp updated this revision to Diff 258288.Apr 17 2020, 4:31 AM
dp edited the summary of this revision. (Show Details)

Updated as suggested by Paul and Matt. Thank you for your help!

dp marked 4 inline comments as done.Apr 17 2020, 4:33 AM
probinson accepted this revision.Apr 17 2020, 5:07 AM

I suggest using past tense in the description, because commentary should describe the code as it is (after the patch).
Ex: llvm assembler handled ... it forced ... is_stmt was specified ...

LGTM aside from that.

This revision is now accepted and ready to land.Apr 17 2020, 5:07 AM
dp edited the summary of this revision. (Show Details)Apr 17 2020, 6:34 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 20 2020, 4:16 AM