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

[dp]

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.

[arsenm]

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

llvm/test/MC/AsmParser/directive_loc_2.s

1

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?

[dp]

llvm/test/MC/AsmParser/directive_loc_2.s

1

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

[aprantl]

llvm/test/MC/AsmParser/directive_loc_2.s

1

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

[dp]

llvm/test/MC/AsmParser/directive_loc_2.s

1

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

[probinson]

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

18

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

24

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.

[probinson]

+ llvm-commits

[dp]

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

[probinson]

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.

llvm/lib/MC/MCParser/AsmParser.cpp

bool AsmParser::parseDirectiveLoc() {
   int64_t ColumnPos = 0;
   if (getLexer().is(AsmToken::Integer)) {
     ColumnPos = getTok().getIntVal();
     if (ColumnPos < 0)
       return TokError("column position less than zero in '.loc' directive");
     Lex();
   }
 
-  unsigned Flags = DWARF2_LINE_DEFAULT_IS_STMT ? DWARF2_FLAG_IS_STMT : 0;
+  auto PrevFlags = getContext().getCurrentDwarfLoc().getFlags();
+  unsigned Flags = PrevFlags & DWARF2_FLAG_IS_STMT;
   unsigned Isa = 0;
   int64_t Discriminator = 0;
 
   auto parseLocOp = [&]() -> bool {
     StringRef Name;
     SMLoc Loc = getTok().getLoc();
     if (parseIdentifier(Name))
       return TokError("unexpected token in '.loc' directive");

llvm/lib/MC/MCParser/MasmParser.cpp

bool MasmParser::parseDirectiveLoc() {
   int64_t ColumnPos = 0;
   if (getLexer().is(AsmToken::Integer)) {
     ColumnPos = getTok().getIntVal();
     if (ColumnPos < 0)
       return TokError("column position less than zero in '.loc' directive");
     Lex();
   }
 
-  unsigned Flags = DWARF2_LINE_DEFAULT_IS_STMT ? DWARF2_FLAG_IS_STMT : 0;
+  auto PrevFlags = getContext().getCurrentDwarfLoc().getFlags();
+  unsigned Flags = PrevFlags & DWARF2_FLAG_IS_STMT;
   unsigned Isa = 0;
   int64_t Discriminator = 0;
 
   auto parseLocOp = [&]() -> bool {
     StringRef Name;
     SMLoc Loc = getTok().getLoc();
     if (parseIdentifier(Name))
       return TokError("unexpected token in '.loc' directive");

llvm/test/MC/AsmParser/directive_loc_2.s

+# RUN: llvm-mc -triple i386-unknown-unknown -filetype=obj %s -o %t
+# RUN: llvm-dwarfdump -debug-info -debug-line %t | FileCheck %s
+
+        .file 1 "test.c"
+        .loc 1 2
+        nop
+        .loc 1 4 is_stmt 0
+        nop
+        .loc 1 6
+        nop
+        .loc 1 8 is_stmt 1
+        nop
+        .loc 1 10
+        nop
+
+
+# CHECK: .debug_info
+# CHECK-EMPTY:
+# CHECK_NEXT: .debug_line
+# CHECK: file_names[ 1]:
+# CHECK-NEXT: name: "test.c"
+# CHECK-NEXT: dir_index: 0
+# CHECK: 0x{{0+}}0 2 0 1 0 0 is_stmt
+# CHECK: 0x{{0+}}1 4 0 1 0 0
+# CHECK-NOT: is_stmt
+# CHECK: 0x{{0+}}2 6 0 1 0 0
+# CHECK-NOT: is_stmt
+# CHECK: 0x{{0+}}3 8 0 1 0 0 is_stmt
+# CHECK: 0x{{0+}}4 10 0 1 0 0 is_stmt
+# CHECK: 0x{{0+}}5 10 0 1 0 0 is_stmt end_sequence