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

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo accepted this revision.
mstorsjo added a comment.
This revision is now accepted and ready to land.

In D55900#1336844 <https://reviews.llvm.org/D55900#1336844>, @rnk wrote:

> In D55900#1336824 <https://reviews.llvm.org/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).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55900/new/

https://reviews.llvm.org/D55900