Permit multiple .file directives with -g

[bcain]

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.

[bcain]

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]

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]

In D54348#1295895, @dblaikie wrote:

...

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?

[probinson]

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]

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?

[bcain]

In D54348#1531741, @cmtice wrote:

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]

In D54348#1531751, @bcain wrote:

In D54348#1531741, @cmtice wrote:

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". :-)

[probinson]

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]

Obsoleted by r364039.

llvm/lib/MC/MCDwarf.cpp

MCDwarfLineTableHeader::tryGetFile(StringRef &Directory,
   }
   // Make space for this FileNumber in the MCDwarfFiles vector if needed.
   if (FileNumber >= MCDwarfFiles.size())
     MCDwarfFiles.resize(FileNumber + 1);
 
   // Get the new MCDwarfFile slot for this FileNumber.
   MCDwarfFile &File = MCDwarfFiles[FileNumber];
 
-  // It is an error to see the same number more than once.
-  if (!File.Name.empty())
-    return make_error<StringError>("file number already allocated",
-                                   inconvertibleErrorCode());
-
   // If any files have embedded source, they all must.
   if (HasSource != (Source != None))
     return make_error<StringError>("inconsistent use of embedded source",
                                    inconvertibleErrorCode());
 
   if (Directory.empty()) {
     // Separate the directory part from the basename of the FileName.
     StringRef tFileName = sys::path::filename(FileName);

llvm/lib/MC/MCParser/AsmParser.cpp

while (!parseOptionalToken(AsmToken::EndOfStatement)) {
     } else {
       return TokError("unexpected token in '.file' directive");
     }
   }
 
   if (FileNumber == -1)
     getStreamer().EmitFileDirective(Filename);
   else {
-    // In case there is a -g option as well as debug info from directive .file,
-    // we turn off the -g option, directly use the existing debug info instead.
-    // Also reset any implicit ".file 0" for the assembler source.
-    if (Ctx.getGenDwarfForAssembly()) {
-      Ctx.getMCDwarfLineTable(0).resetRootFile();
-      Ctx.setGenDwarfForAssembly(false);
-    }
-
     MD5::MD5Result *CKMem = nullptr;
     if (HasMD5) {
       CKMem = (MD5::MD5Result *)Ctx.allocate(sizeof(MD5::MD5Result), 1);
       for (unsigned i = 0; i != 8; ++i) {
         CKMem->Bytes[i] = uint8_t(MD5Hi >> ((7 - i) * 8));
         CKMem->Bytes[i + 8] = uint8_t(MD5Lo >> ((7 - i) * 8));
       }
     }

llvm/test/MC/AsmParser/directive_file-4.s

+// RUN: llvm-mc -g -triple i386-unknown-unknown  %s | FileCheck -check-prefix=CHECK-DEBUG-ASM %s
+// RUN: llvm-mc -triple i386-unknown-unknown  %s | FileCheck -check-prefix=CHECK-DEFAULT %s
+// RUN: llvm-mc -g -triple i386-unknown-unknown %s -filetype=obj | obj2yaml | FileCheck -check-prefix=CHECK-DEBUG-OBJ %s
+
+// Test for Bug 38994
+// This testcase has a single function and two .file directives:
+// one with the [file-num] argument and one without.
+// When compiled with -g, this testcase will not report an error,
+// and generate new debug info.  The '.file 1' directive will be
+// replaced by the assembler-generated '.file 1' directive.
+
+        .file "xyz.c"
+f1:
+        .file 1 "abc.c"
+        nop
+.size f1, .-f1
+
+// CHECK-DEFAULT: .file "xyz.c"
+// CHECK-DEFAULT-NEXT: f1:
+// CHECK-DEFAULT-NEXT: .file 1 "abc.c"
+
+// CHECK-DEBUG-ASM: .file "xyz.c"
+// CHECK-DEBUG-ASM-NEXT: f1:
+// CHECK-DEBUG-ASM-NEXT: .file 1 {{.*}} "directive_file-4.s"
+
+// CHECK-DEBUG-OBJ:  Sections:
+// CHECK-DEBUG-OBJ:  - Name:            .text
+// CHECK-DEBUG-OBJ:  - Name:            .debug_info
+// CHECK-DEBUG-OBJ:  - Name:            .rel.debug_info
+// CHECK-DEBUG-OBJ:    Info:            .debug_info
+// CHECK-DEBUG-OBJ:  Symbols:

llvm/test/MC/AsmParser/directive_file-5.s

+// RUN: not llvm-mc -g -triple i386-unknown-unknown  %s | FileCheck -check-prefix=CHECK-DEBUG %s
+// RUN: llvm-mc  -triple i386-unknown-unknown %s -filetype=obj | obj2yaml | FileCheck -check-prefix=CHECK-DEFAULT %s
+
+// Test for Bug 38994
+// This testcase .... TBD
+
+        .file "xyz.c"
+.text
+
+f1:
+        nop
+        .file 1 "abc.c"
+
+// CHECK-DEBUG: error
+
+// CHECK-DEFAULT-NOT:  - Name:            .debug_info