[COFF] support /ignore:4217

[inglorion]

Summary

lld-link accepts link.exe's /ignore option, but used to ignore
it. This can lead to semantic differences when warnings are treated as
fatal errors. One such case is when we resolve an __imp_ symbol to a
local definition. We emit a warning in that case, which /wx turns into
a fatal. This change makes lld-link accept /ignore:4217 to suppress
that warning, so that code that links with link.exe /wx /ignore:4217
links with lld-link, too.

Fixes PR35762.

[inglorion]

fix description of test

[ruiu]

LGTM

lld/COFF/Driver.cpp

799–800

nit: remove {}

lld/COFF/SymbolTable.cpp

120

remove {}

133

ditto

[inglorion]

removed braces as per @ruiu's suggestions

[smeenai]

General question: it looks like a lot of COFF tests are written using yaml2obj, whereas ELF always uses assembly files and llvm-mc (and I've been following that pattern for the few COFF patches I've written as well). Is there an actual preference for yaml2obj tests in COFF, or is it just a historic artifact? I personally find assembly tests a lot easier to understand.

[rnk]

In D41606#964821, @smeenai wrote:

General question: it looks like a lot of COFF tests are written using yaml2obj, whereas ELF always uses assembly files and llvm-mc (and I've been following that pattern for the few COFF patches I've written as well). Is there an actual preference for yaml2obj tests in COFF, or is it just a historic artifact? I personally find assembly tests a lot easier to understand.

I agree, I'd recommend using .s files for all non-PDB tests. I use yaml mostly for objects with codeview because it @zturner added a great symbolic representation for codeview.

However, our COFF yaml isn't a very good representation for code and relocations. Code is represented as a binary blob and relocations are stored on the side. Storing relocations separately is a problem for codeview symbol records because if you want to add one record, you have to determine its size and update the offsets of all the relocations after the symbol. Type records don't have this problem because they do not contain relocations. Type merging is significantly more complicated than symbol linking, so we naturally have more tests for it, and increasing the testability of symbol records hasn't been as much of a priority.

The best way to unify these tests would probably be to add more directives to MC to emit complete codeview records. We can still test invalid records by falling back to open coding them with .byte, .short, and .long data directives.

[mstorsjo]

In D41606#964821, @smeenai wrote:

General question: it looks like a lot of COFF tests are written using yaml2obj, whereas ELF always uses assembly files and llvm-mc (and I've been following that pattern for the few COFF patches I've written as well). Is there an actual preference for yaml2obj tests in COFF, or is it just a historic artifact? I personally find assembly tests a lot easier to understand.

I also agree that .s tests are easier. I remember some of the tests I've added recently where I despite that ended up using yaml2obj for features that weren't quite as easy to express in .s form.

[smeenai]

In D41606#964885, @mstorsjo wrote:

In D41606#964821, @smeenai wrote:

General question: it looks like a lot of COFF tests are written using yaml2obj, whereas ELF always uses assembly files and llvm-mc (and I've been following that pattern for the few COFF patches I've written as well). Is there an actual preference for yaml2obj tests in COFF, or is it just a historic artifact? I personally find assembly tests a lot easier to understand.

I also agree that .s tests are easier. I remember some of the tests I've added recently where I despite that ended up using yaml2obj for features that weren't quite as easy to express in .s form.

Yup, there are definitely tests that aren't easily expressed (or aren't expressible at all) in assembly in COFF; I think ELF assembly directives are a bit richer (and clang's COFF assembly is still more expressive than MSVC's). I do think it's worthwhile to use them where they work though; for example, I believe the test in this file could be expressed as something like:

.globl foo
foo:
        ret

.globl main
main:
        jmp     *__imp_foo(%rip)

lld/COFF/Config.h

struct Configuration {
   bool AllowBind = true;
   bool NxCompat = true;
   bool AllowIsolation = true;
   bool TerminalServerAware = true;
   bool LargeAddressAware = false;
   bool HighEntropyVA = false;
   bool AppContainer = false;
   bool MinGW = false;
+  bool WarnLocallyDefinedImported = true;
 };
 
 extern Configuration *Config;
 
 } // namespace coff
 } // namespace lld
 
 #endif

lld/COFF/Driver.cpp

void LinkerDriver::link(ArrayRef<const char *> ArgsArr) {
   }
 
   // Construct search path list.
   SearchPaths.push_back("");
   for (auto *Arg : Args.filtered(OPT_libpath))
     SearchPaths.push_back(Arg->getValue());
   addLibSearchPaths();
 
+  // Handle /ignore
+  for (auto *Arg : Args.filtered(OPT_ignore)) {
+    if (StringRef(Arg->getValue()) == "4217") {
+      Config->WarnLocallyDefinedImported = false;
+    }
+    // Other warning numbers are ignored.
+  }
+
   // Handle /out
   if (auto *Arg = Args.getLastArg(OPT_out))
     Config->OutputFile = Arg->getValue();
 
   // Handle /verbose
   if (Args.hasArg(OPT_verbose))
     Config->Verbose = true;
   errorHandler().Verbose = Config->Verbose;

lld/COFF/Options.td

 def delayload : P<"delayload", "Delay loaded DLL name">;
 def entry   : P<"entry", "Name of entry point symbol">;
 def errorlimit : P<"errorlimit",
     "Maximum number of errors to emit before stopping (0 = no limit)">;
 def export  : P<"export", "Export a function">;
 // No help text because /failifmismatch is not intended to be used by the user.
 def failifmismatch : P<"failifmismatch", "">;
 def heap    : P<"heap", "Size of the heap">;
+def ignore : P<"ignore", "Specify warning codes to ignore">;
 def implib  : P<"implib", "Import library name">;
 def libpath : P<"libpath", "Additional library search path">;
 def linkrepro : P<"linkrepro", "Dump linker invocation and input files for debugging">;
 def lldltocache : P<"lldltocache", "Path to ThinLTO cached object file directory">;
 def lldltocachepolicy : P<"lldltocachepolicy", "Pruning policy for the ThinLTO cache">;
 def lldsavetemps : F<"lldsavetemps">,
     HelpText<"Save temporary files instead of deleting them">;
 def machine : P<"machine", "Specify target platform">;

 def nologo : F<"nologo">;
 def throwingnew : F<"throwingnew">;
 def editandcontinue : F<"editandcontinue">;
 def fastfail : F<"fastfail">;
 
 def delay : QF<"delay">;
 def errorreport : QF<"errorreport">;
 def idlout : QF<"idlout">;
-def ignore : QF<"ignore">;
 def maxilksize : QF<"maxilksize">;
 def natvis : QF<"natvis">;
 def pdbaltpath : QF<"pdbaltpath">;
 def tlbid : QF<"tlbid">;
 def tlbout : QF<"tlbout">;
 def verbose_all : QF<"verbose">;
 def guardsym : QF<"guardsym">;

lld/COFF/SymbolTable.cpp

void SymbolTable::reportRemainingUndefines() {
   }
 
   if (Undefs.empty() && LocalImports.empty())
     return;
 
   for (Symbol *B : Config->GCRoot) {
     if (Undefs.count(B))
       errorOrWarn("<root>: undefined symbol: " + B->getName());
+    if (Config->WarnLocallyDefinedImported) {
       if (Symbol *Imp = LocalImports.lookup(B))
         warn("<root>: locally defined symbol imported: " + Imp->getName() +
              " (defined in " + toString(Imp->getFile()) + ")");
     }
+  }
 
   for (ObjFile *File : ObjFile::Instances) {
     for (Symbol *Sym : File->getSymbols()) {
       if (!Sym)
         continue;
       if (Undefs.count(Sym))
         errorOrWarn(toString(File) + ": undefined symbol: " + Sym->getName());
+      if (Config->WarnLocallyDefinedImported) {
         if (Symbol *Imp = LocalImports.lookup(Sym))
           warn(toString(File) + ": locally defined symbol imported: " +
-             Imp->getName() + " (defined in " + toString(Imp->getFile()) + ")");
+               Imp->getName() + " (defined in " + toString(Imp->getFile()) +
+               ")");
+      }
     }
   }
 }
 
 std::pair<Symbol *, bool> SymbolTable::insert(StringRef Name) {
   Symbol *&Sym = SymMap[CachedHashStringRef(Name)];
   if (Sym)
     return {Sym, false};

lld/test/COFF/ignore4217.yaml

+# Tests that /ignore:4217 suppresses "locally defined symbol imported" warnings.
+# RUN: yaml2obj < %s > %t.obj
+
+# RUN: lld-link -entry:main -out:%t.exe %t.obj 2>&1 \
+# RUN:     | FileCheck -check-prefix=WARNINGS %s
+# RUN: lld-link -ignore:4217 -entry:main -out:%t.exe %t.obj 2>&1 \
+# RUN:     | FileCheck -allow-empty -check-prefix=SUPPRESSED %s
+
+# WARNINGS: locally defined symbol imported
+# SUPPRESSED-NOT: locally defined symbol imported
+
+--- !COFF
+header:
+  Machine:         IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [  ]
+sections:
+  - Name:            .text
+    Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]
+    Alignment:       16
+    SectionData:     B82A000000C3662E0F1F8400000000004883EC28C744242400000000E800000000904883C428C3
+    Relocations:
+      - VirtualAddress:  29
+        SymbolName:      __imp_foo
+        Type:            IMAGE_REL_AMD64_REL32
+  - Name:            .data
+    Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
+    Alignment:       4
+    SectionData:     ''
+symbols:
+  - Name:            .text
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+    SectionDefinition:
+      Length:          39
+      NumberOfRelocations: 1
+      NumberOfLinenumbers: 0
+      CheckSum:        3087210877
+      Number:          1
+  - Name:            .data
+    Value:           0
+    SectionNumber:   2
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+    SectionDefinition:
+      Length:          0
+      NumberOfRelocations: 0
+      NumberOfLinenumbers: 0
+      CheckSum:        0
+      Number:          2
+  - Name:            foo
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_FUNCTION
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+  - Name:            main
+    Value:           16
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_FUNCTION
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+  - Name:            __imp_foo
+    Value:           0
+    SectionNumber:   0
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+...