[all-commits] [llvm/llvm-project] 89efff: [LTO] [LLD] Don't alias the __imp_func and func sy...

[Martin Storsjö via All-commits]

  Branch: refs/heads/main
  Home:   https://github.com/llvm/llvm-project
  Commit: 89efffd463ecc39cb17440ef7bb66d26141322aa
      https://github.com/llvm/llvm-project/commit/89efffd463ecc39cb17440ef7bb66d26141322aa
  Author: Martin Storsjö <martin at martin.st>
  Date:   2023-11-21 (Tue, 21 Nov 2023)

  Changed paths:
    M lld/COFF/SymbolTable.cpp
    M lld/test/COFF/lto-dllimport.ll
    M lld/test/COFF/lto-imp-prefix.ll
    M llvm/lib/LTO/LTO.cpp

  Log Message:
  -----------
  [LTO] [LLD] Don't alias the __imp_func and func symbol resolutions (#71376)

Commit b963c0b658cc54b370832df4f5a3d63fd69da334 fixed LTO compilation of
cases where one translation unit is calling a function with the
dllimport attribute, and another translation unit provides this function
locally within the same linked module (i.e. not actually dllimported);
see https://github.com/llvm/llvm-project/issues/37453 or
https://bugs.llvm.org/show_bug.cgi?id=38105 for full context.

This was fixed by aliasing their GlobalResolution structs, for the
`__imp_` prefixed and non prefixed symbols.

I believe this fix to be wrong.

This patch reverts that fix, and fixes the same issue differently,
within LLD instead.

The fix assumed that one can treat the `__imp_` prefixed and unprefixed
symbols as equal, referencing SVN r240620
(d766653534e0cff702e42a43b44d3057b6094fea). However that referenced
commit had mistaken how this logic works, which was corrected later in
SVN r240622 (88e0f9206b4dccb56dee931adab08f89ff80525a); those symbols
aren't direct aliases for each other - but if there's a need for the
`__imp_` prefixed one and the other one exists, the `__imp_` prefixed
one is created, as a pointer to the other one.

However this fix only works if both translation units are compiled as
LTO; if the caller is compiled as a regular object file and the callee
is compiled as LTO, the fix fails, as the LTO compilation doesn't know
that the unprefixed symbol is needed.

The only level that knows of the potential relationship between the
`__imp_` prefixed and unprefixed symbol, across regular and bitcode
object files, is LLD itself.

Therefore, revert the original fix from
b963c0b658cc54b370832df4f5a3d63fd69da334, and fix the issue differently
- when concluding that we can fulfill an undefined symbol starting with
`__imp_`, mark the corresponding non prefixed symbol as used in a
regular object for the LTO compilation, to make sure that this non
prefixed symbol exists after the LTO compilation, to let LLD do the
fixup of the local import.

Extend the testcase to test a regular object file calling an LTO object
file, which previously failed.

This change also fixes another issue; an object file can provide both
unprefixed and prefixed versions of the same symbol, like this:

    void importedFunc(void) { 
    }
    void (*__imp_importedFunc)(void) = importedFunc;

That allows the function to be called both with and without dllimport
markings. (The concept of automatically resolving a reference to
`__imp_func` to a locally defined `func` only is done in MSVC style
linkers, but not in GNU ld, therefore MinGW mode code often uses this
construct.)

Previously, the aliasing of global resolutions at the LTO level would
trigger a failed assert with "Multiple prevailing defs are not allowed"
for this case, as both `importedFunc` and `__imp_importedFunc` could be
prevailing. Add a case to the existing LLD test case lto-imp-prefix.ll
to test this as well.

This change (together with previous change in
3ab6209a3f93bdbeec8e9b9fcc00a9a4980915ff) completes LLD to work with
mingw-w64-crt files (the base glue code for a mingw-w64 toolchain) built
with LTO.