[MC] Enable .file support on COFF and diagnose it on unsupported targets

[rnk]

Summary

The "single parameter" .file directive appears to be an ELF-only feature
that is intended to insert the main source filename into the string
table table.

I noticed that if you assemble an ELF .s file for COFF, typically it
will assert right away on a .file directive near the top of the file. My
first change was to make this emit a proper error in the asm parser so
that we don't assert so easily.

However, COFF actually does have some support for this directive, and if
you emit an object file, llvm-mc does not assert. When emitting a COFF
object, MC will take those file names and create "debug" symbol table
entries for them. I'm not familiar with these kinds of symbol table
entries, and I'm not aware of any users of them, but @compnerd added
them a while ago. They don't introduce absolute paths, and most main
source file paths are short enough that this extra entry shouldn't cause
any problems, so I enabled the flag in MCAsmInfoCOFF that indicates that
it's supported.

This has the side effect of adding an extra debug symbol to every object
produced by clang, which is a pretty big functional change. My question
is, should we keep the functionality or remove it in the name of symbol
table minimalism?

[mstorsjo]

However, COFF actually does have some support for this directive, and if you emit an object file, llvm-mc does not assert.

I'm not sure I'm following here - if you emit a COFF object file, from what source? From normal LLVM IR CodeGen, or from MC assembly with a .file directive?

When emitting a COFF object, MC will take those file names and create "debug" symbol table entries for them.

In which cases are these emitted - only if debug info is enabled?

Also I'm a bit confused by the test added - doesn't this test check that it errors out, while the flag that is set to true in MCAsmInfoCOFF makes this directive be handled so it shouldn't error out after all?

[rnk]

In D55900#1336824, @mstorsjo wrote:

However, COFF actually does have some support for this directive, and if you emit an object file, llvm-mc does not assert.

I'm not sure I'm following here - if you emit a COFF object file, from what source? From normal LLVM IR CodeGen, or from MC assembly with a .file directive?

From a .s file with a .file directive.

When emitting a COFF object, MC will take those file names and create "debug" symbol table entries for them.

In which cases are these emitted - only if debug info is enabled?

It seems LLVM's AsmPrinter always emits .file if the target supports it, regardless of debug info. If the directive is emitted, then the file appears in the symbol table.

Also I'm a bit confused by the test added - doesn't this test check that it errors out, while the flag that is set to true in MCAsmInfoCOFF makes this directive be handled so it shouldn't error out after all?

The test is for MachO, which would crash without the new diagnostic code. The new diagnostic code breaks an existing COFF test (llvm/test/MC/COFF/file.s) unless we change COFFMCAsmInfo to support .file.

[mstorsjo]

In D55900#1336844, @rnk wrote:

In D55900#1336824, @mstorsjo wrote:

However, COFF actually does have some support for this directive, and if you emit an object file, llvm-mc does not assert.

I'm not sure I'm following here - if you emit a COFF object file, from what source? From normal LLVM IR CodeGen, or from MC assembly with a .file directive?

From a .s file with a .file directive.

Ah, I managed to see the difference now - I presume you mean the difference in running llvm-mc with or without -filetype obj.

When emitting a COFF object, MC will take those file names and create "debug" symbol table entries for them.

In which cases are these emitted - only if debug info is enabled?

It seems LLVM's AsmPrinter always emits .file if the target supports it, regardless of debug info. If the directive is emitted, then the file appears in the symbol table.

Also I'm a bit confused by the test added - doesn't this test check that it errors out, while the flag that is set to true in MCAsmInfoCOFF makes this directive be handled so it shouldn't error out after all?

The test is for MachO, which would crash without the new diagnostic code. The new diagnostic code breaks an existing COFF test (llvm/test/MC/COFF/file.s) unless we change COFFMCAsmInfo to support .file.

Ah, I missed that fact - then it does indeed make sense.

This has the side effect of adding an extra debug symbol to every object produced by clang, which is a pretty big functional change. My question is, should we keep the functionality or remove it in the name of symbol table minimalism?

I don't mind keeping it, as I presume it doesn't travel past the linker anyway (or if it ends up linked in debug info it doesn't matter).

llvm/trunk/lib/MC/MCAsmInfoCOFF.cpp

 void MCAsmInfoCOFF::anchor() {}
 
 MCAsmInfoCOFF::MCAsmInfoCOFF() {
   // MingW 4.5 and later support .comm with log2 alignment, but .lcomm uses byte
   // alignment.
   COMMDirectiveAlignmentIsInBytes = false;
   LCOMMDirectiveAlignmentType = LCOMM::ByteAlignment;
   HasDotTypeDotSizeDirective = false;
-  HasSingleParameterDotFile = false;
+  HasSingleParameterDotFile = true;
   WeakRefDirective = "\t.weak\t";
   HasLinkOnceDirective = true;
 
   // Doesn't support visibility:
   HiddenVisibilityAttr = HiddenDeclarationVisibilityAttr = MCSA_Invalid;
   ProtectedVisibilityAttr = MCSA_Invalid;
 
   // Set up DWARF directives

llvm/trunk/lib/MC/MCParser/AsmParser.cpp

if (Keyword == "md5") {
                 "unexpected token in '.file' directive") ||
           parseEscapedString(SourceString))
         return true;
     } else {
       return TokError("unexpected token in '.file' directive");
     }
   }
 
-  if (FileNumber == -1)
+  if (FileNumber == -1) {
+    if (!getContext().getAsmInfo()->hasSingleParameterDotFile())
+      return Error(DirectiveLoc,
+                   "target does not support '.file' without a number");
     getStreamer().EmitFileDirective(Filename);
-  else {
+  } 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);
     }
 

llvm/trunk/test/MC/COFF/file.s

 // RUN: llvm-mc -triple i686-windows -filetype obj %s -o - | llvm-objdump -t - \
 // RUN:   | FileCheck %s
 
+// Round trip through .s output to exercise MCAsmStreamer.
+// RUN: llvm-mc -triple i686-windows %s -o - \
+// RUN:   | llvm-mc -triple i686-windows - -filetype=obj -o - | llvm-objdump -t - \
+// RUN:   | FileCheck %s
+
 // RUN: llvm-mc -triple i686-windows -filetype obj %s -o - \
 // RUN:   | llvm-readobj -symbols | FileCheck %s -check-prefix CHECK-SCN
 
   .file "null-padded.asm"
 // CHECK: (nx 1) {{0x[0-9]+}} .file
 // CHECK-NEXT: AUX null-padded.asm{{$}}
 
   .file "eighteen-chars.asm"

llvm/trunk/test/MC/MachO/file-single.s

+// RUN: not llvm-mc -triple i386-apple-darwin9 %s -o /dev/null 2>&1 | FileCheck %s
+
+// Previously this crashed MC.
+
+// CHECK: error: target does not support '.file' without a number
+
+        .file "dir/foo"
+        nop