[LLD] [COFF] Improve the autoexport check for symbols from import libraries with -opt:noref

[mstorsjo]

Summary

If -opt:noref is specified, they can end up with isLive() == 1 when the autoexport check is run.

To reduce the risk of potential issues, only consider exporting DefinedRegular and DefinedCommon, nothing else.

[rnk]

Do you mind digging a little bit so we can get a test case for it? Is there some way to get symbol definitions from .drectives like ELF with --defsym or something?

[mstorsjo]

In D40014#925289, @rnk wrote:

Do you mind digging a little bit so we can get a test case for it? Is there some way to get symbol definitions from .drectives like ELF with --defsym or something?

Ok, I can try to dig closer and see where it actually is coming from.

[mstorsjo]

Identified the actual root cause, improved the related code further by checking for specific types of Defined symbols, added a test.

[rnk]

lgtm, definitely don't want to auto-export that imported data. The absolute symbol thing can be a follow-up change.

COFF/MinGW.cpp

94 ↗

(On Diff #122991)

These are the Defined symbol kinds:

DefinedRegularKind = 0,
DefinedCommonKind,
DefinedLocalImportKind,
DefinedImportThunkKind,
DefinedImportDataKind,
DefinedAbsoluteKind,
DefinedSyntheticKind,

I can't think of any synthetic symbols that we'd want to auto-export, but it's definitely possible to define an absolute symbol in an object file that should be auto-exported, right? The linker-generated absolute symbols will probably fail the getFile() null check below.

[mstorsjo]

In D40014#926637, @rnk wrote:

lgtm, definitely don't want to auto-export that imported data. The absolute symbol thing can be a follow-up change.

I can amend it to allow absolute symbols before committing - I wasn't quite sure what they were originally, but I guess it can make sense to export them as well.

[pcc]

I didn't think that the pe format allowed absolute symbols to be exported.

[ruiu]

Please add comments to describe new code.

[mstorsjo]

In D40014#926966, @ruiu wrote:

Please add comments to describe new code.

Ok, maybe like this?

// Only allow the symbol kinds that make sense to export;
// in particular, disallow import symbols.
if (!isa<DefinedRegular>(Sym) && !isa<DefinedCommon>(Sym))

And

// Check that file is non-null before dereferencing it,
// symbols not originating in regular object files probably shouldn't be exported.
if (!Sym->getFile())

[ruiu]

Sounds good.

lld/trunk/COFF/MinGW.cpp

ExcludeObjects = {
       "crtbegin.o",
       "crtend.o",
   };
 }
 
 bool AutoExporter::shouldExport(Defined *Sym) const {
   if (!Sym || !Sym->isLive() || !Sym->getChunk())
     return false;
+  // Only allow the symbol kinds that make sense to export; in particular,
+  // disallow import symbols.
+  if (!isa<DefinedRegular>(Sym) && !isa<DefinedCommon>(Sym))
+    return false;
   if (ExcludeSymbols.count(Sym->getName()))
     return false;
+  // Check that file is non-null before dereferencing it, symbols not
+  // originating in regular object files probably shouldn't be exported.
+  if (!Sym->getFile())
+    return false;
   StringRef LibName = sys::path::filename(Sym->getFile()->ParentName);
   // Drop the file extension.
   LibName = LibName.substr(0, LibName.rfind('.'));
   if (ExcludeLibs.count(LibName))
     return false;
   StringRef FileName = sys::path::filename(Sym->getFile()->getName());
   if (LibName.empty() && ExcludeObjects.count(FileName))
     return false;

lld/trunk/test/COFF/export-all.s

 # RUN: llvm-mc -triple=x86_64-windows-gnu %T/libs/crtfunc.s -filetype=obj -o %T/libs/crt2.o
 # RUN: lld-link -out:%t.dll -dll -entry:DllMainCRTStartup %t.main.obj -lldmingw %T/libs/crt2.o %T/libs/libmingwex.a -output-def:%t.def
 # RUN: echo "EOF" >> %t.def
 # RUN: cat %t.def | FileCheck -check-prefix=CHECK-EXCLUDE %s
 
 # CHECK-EXCLUDE: EXPORTS
 # CHECK-EXCLUDE-NEXT: foobar @1
 # CHECK-EXCLUDE-NEXT: EOF
+
+# Test that we handle import libraries together with -opt:noref.
+
+# RUN: yaml2obj < %p/Inputs/hello32.yaml > %t.obj
+# RUN: lld-link -lldmingw -dll -out:%t.dll -entry:main@0 %t.obj -implib:%t.lib -opt:noref %p/Inputs/std32.lib -output-def:%t.def
+# RUN: echo "EOF" >> %t.def
+# RUN: cat %t.def | FileCheck -check-prefix=CHECK-IMPLIB %s
+
+# CHECK-IMPLIB: EXPORTS
+# CHECK-IMPLIB-NEXT: main@0 @1
+# CHECK-IMPLIB-NEXT: EOF