Add an alias object to the import lib #2734

barcharcraz · 2022-05-20T03:51:55Z

We used to use an internal "aliasobj" tool to generate this object, but it would be a ton of work to release it. Thus, we switched to using /ALTERNATENAME, however we put that in the directives section of xonce2.cpp with a static init function call pointing there from shared mutex. This was a problem because if you compile with an old toolset but later link with a new toolset the linker has no way of knowing it should be pulling in the new xonce2.cpp.

This keeps the /ALTERNATENAME directives (though they could later be removed, I suppose) but additionally adds an alias object (alias.asm.obj) to the import library that contains "real" weak external symbols.

This generates an object file that's very similar to what aliasobj would have done, except there's only one object, instead of one per symbol (I think this is fine), and the object is more "complete" (it contains debug info, and a .text section (that's empty) and so on).

The cmake has been updated quite extensively to specify options for the correct language. I only bothered to do this for the options that actually make it to the implib objects compilation.

A simple test has been added, this test uses it own prefix with just the /Mxx options.

Fixes #2655

This is enabled on x86 and x64 only, as it relies on a MASM assembly file, still, it's not actually assembly, there's no _code_ generated, just objects with the correct sections. whoops fix

…ze without doing anything special.

This is so the mangling is right.

CMakeLists.txt

stl/CMakeLists.txt

stl/src/alias.asm

tests/std/tests/GH_002655_alternate_name_broke_linker/test.cpp

…ROTO aliasing.

CaseyCarter

This is "approve with suggestions": I wouldn't be upset if we merge and cleanup my nitpicks later since this is high priority.

stl/src/alias.asm

CMakeLists.txt

tests/std/tests/GH_002655_alternate_name_broke_linker/test.cpp

stl/CMakeLists.txt

stl/src/alias.asm

tests/std/tests/GH_002655_alternate_name_broke_linker/test.cpp

stl/src/alias.asm

CMakeLists.txt

StephanTLavavej · 2022-05-21T01:44:46Z

@CaseyCarter I pushed minor changes after you approved.

@barcharcraz Thank you for fixing this! 😻 I had no idea the assembler could be used to achieve this.

StephanTLavavej · 2022-05-21T02:06:23Z

I have filed https://gitlab.kitware.com/cmake/cmake/-/issues/23537 'MSVC assembler shows "Assembling" messages that should be suppressed'.

StephanTLavavej · 2022-05-21T03:00:15Z

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

StephanTLavavej · 2022-05-21T22:25:38Z

✅ 🎉 😸

I have verified the end-to-end scenario for both x86 and x64. I built a program using std::call_once() with the 16.11 toolset. Attempting to perform the final link with 17.3 Preview 1 failed with the unresolved externals, and it succeeded with this PR. (I made sure to consume the 16.11-built object file, and it printed that it was built with _MSC_VER == 1929, so I definitely wasn't compiling it fresh.)

StephanTLavavej · 2022-05-21T22:41:57Z

I've pushed changes to add this to MSBuild.

StephanTLavavej · 2022-05-24T01:07:07Z

Trivial adjacent-add conflict in CMakeLists.txt, placed the WinSDK check before the architecture check.
Trivial adjacent-add conflict in tests/std/test.lst.

StephanTLavavej · 2022-05-24T01:38:46Z

Thanks for fixing this major bug that I wrote! 🐞 😻 🛠️

Co-authored-by: Stephan T. Lavavej <stl@microsoft.com>

jlaanstra · 2022-10-21T04:42:17Z

stl/msbuild/stl_base/stl.files.settings.targets

@@ -176,6 +176,18 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
        </BuildFiles>
    </ItemGroup>

+    <ItemGroup Condition="'$(BuildArchitecture)' == 'i386' or '$(BuildArchitecture)' == 'amd64'">


Seems like this only fixes this for x86 and x64 but not for arm64?

@StephanTLavavej

The aliases are not needed on ARM64 IIRC, this is for backwards compatibility on x86/x64 with code that predates ARM64 support. Are you having an issue building some ARM64 code?

Yes I have the same LNK2019: unresolved external symbol errors on ARM64 and have applied the AlternateName workaround for arm64 for now.

I'd recommend opening a new issue with repro steps then. Are you targetting ARM64EC with some old x64 binary?

This is a normal ARM64 static library similarly compiled before this change went in, causing the exact same symptoms after this change when included in a dll targeting ARM64.

barcharcraz added 5 commits May 19, 2022 20:40

Add linker aliases for InitOnceBeginInitialize and InitOnceComplete

e83e7e1

This is enabled on x86 and x64 only, as it relies on a MASM assembly file, still, it's not actually assembly, there's no _code_ generated, just objects with the correct sections. whoops fix

Add a test to verify that you can call __std_init_once_begin_initiali…

ff56bea

…ze without doing anything special.

test all the libs

dcd5003

use the correct parameter size for init_once_complete

b4b389d

make the test actually call the function instead of using /INCLUDE

d6e27c6

This is so the mangling is right.

barcharcraz requested a review from a team as a code owner May 20, 2022 03:51

CaseyCarter requested changes May 20, 2022

View reviewed changes

barcharcraz added 4 commits May 20, 2022 14:42

refactor alias.asm to be a little more terse, and no longer rely on P…

473945d

…ROTO aliasing.

add comments for alias.asm

cc15ec1

make VCLIBS_TARGET_ARCHITECTURE lowercase

8e7a7f7

use logical instead of bitwise or

51b6603

barcharcraz requested a review from CaseyCarter May 20, 2022 22:36

CaseyCarter approved these changes May 20, 2022

View reviewed changes

CaseyCarter added bug Something isn't working high priority Important! labels May 20, 2022

barcharcraz added 3 commits May 20, 2022 16:23

add comments to the #endif

e82d1de

clean up comments+formatting in alias.asm

bb1f546

add alternatename test to test.lst

5735a1b

CaseyCarter approved these changes May 20, 2022

View reviewed changes

StephanTLavavej added 2 commits May 20, 2022 17:43

Code review feedback.

66b75f4

Comment cleanups and clarification.

8f1c247

StephanTLavavej reviewed May 21, 2022

View reviewed changes

StephanTLavavej approved these changes May 21, 2022

View reviewed changes

StephanTLavavej self-assigned this May 21, 2022

Add MSBuild changes.

4ae3f4e

CaseyCarter approved these changes May 22, 2022

View reviewed changes

Merge branch 'main' into not_alias_obj

dc375ef

StephanTLavavej merged commit fc9232d into microsoft:main May 24, 2022

fsb4000 pushed a commit to fsb4000/STL that referenced this pull request Aug 13, 2022

Add an alias object to the import lib (microsoft#2734)

18d815f

Co-authored-by: Stephan T. Lavavej <stl@microsoft.com>

jlaanstra reviewed Oct 21, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an alias object to the import lib #2734

Add an alias object to the import lib #2734

barcharcraz commented May 20, 2022

CaseyCarter left a comment

StephanTLavavej commented May 21, 2022

StephanTLavavej commented May 21, 2022

StephanTLavavej commented May 21, 2022

StephanTLavavej commented May 21, 2022

StephanTLavavej commented May 21, 2022

StephanTLavavej commented May 24, 2022

StephanTLavavej commented May 24, 2022

jlaanstra Oct 21, 2022

jlaanstra Oct 21, 2022

sylveon Oct 21, 2022

jlaanstra Oct 21, 2022

sylveon Oct 21, 2022

jlaanstra Oct 21, 2022

Add an alias object to the import lib #2734

Add an alias object to the import lib #2734

Conversation

barcharcraz commented May 20, 2022

CaseyCarter left a comment

Choose a reason for hiding this comment

StephanTLavavej commented May 21, 2022

StephanTLavavej commented May 21, 2022

StephanTLavavej commented May 21, 2022

StephanTLavavej commented May 21, 2022

✅ 🎉 😸

StephanTLavavej commented May 21, 2022

StephanTLavavej commented May 24, 2022

StephanTLavavej commented May 24, 2022

jlaanstra Oct 21, 2022

Choose a reason for hiding this comment

jlaanstra Oct 21, 2022

Choose a reason for hiding this comment

sylveon Oct 21, 2022

Choose a reason for hiding this comment

jlaanstra Oct 21, 2022

Choose a reason for hiding this comment

sylveon Oct 21, 2022

Choose a reason for hiding this comment

jlaanstra Oct 21, 2022

Choose a reason for hiding this comment