Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use /ALTERNATENAME instead of checked-in OBJ files generated by aliasobj #2381

Merged
merged 6 commits into from
Jan 6, 2022

Conversation

StephanTLavavej
Copy link
Member

@StephanTLavavej StephanTLavavej commented Dec 7, 2021

The STL's build currently depends on checked-in OBJ files generated by an internal tool named aliasobj. We've started talking to the compiler back-end team, exploring whether they can make that publicly available at some point, but that may not happen soon. (@AlexGuteniev filed DevCom-1590983 and @BillyONeal filed VSO-1116868.)

In the meantime, we really need to stop using checked-in OBJ files, in order to secure our supply chain. This is my attempt to use the back-end's recommended alternative, the linker's undocumented /ALTERNATENAME option. (This is strictly more accessible to our GitHub contributors than the internal aliasobj tool.)

The main concerns are:

  • Putting #pragma comment into <mutex> might result in user linker invocations being sent a whole bunch of identical directives. I didn't check to see whether this is actually a problem (they might be deduplicated?).
  • ARM64EC and CHPE are weird. I'm currently using the fallback mechanism for them, since that's safe, and the only downside is a bit of performance.
  • Clang doesn't appear to handle #pragma comment when it appears in the STL's object files, so I'm activating the fallback for Clang too. I haven't yet reported that as a bug.
  • I tested this internally and x86 built and passed tests, indicating that /clr still works.

The changes are:

  • Remove the stl/aliases subdirectory, and all of the CMake/MSBuild machinery mentioning it.
  • Add the #pragmas to xonce2.cpp (which is conveniently already in the build system). x86 uses a different mangling from x64/ARM/ARM64; we can unify the latter. For ARM64EC and CHPE, we use the fallbacks. (I had experimental pragmas for them in an early commit but I didn't test them.)
  • Add __std_init_once_link_alternate_names_and_abort(), a function that will allow us to drag in xonce2.cpp without a #pragma in the header. Then the linker will discover the pragmas providing /ALTERNATENAME. By wrapping abort(), we avoid paying any increased costs.
  • In <mutex>, extend the forwarder workaround to ARM64EC, CHPE, and Clang. (I didn't check whether /clr could use /ALTERNATENAME, I simply assumed it couldn't.)

If aliasobj is made publicly available in the future, we can return to the old scheme, building the OBJ files freshly instead of having them be checked-in.

Although it's weird to use meow_clr forwarders in non-/clr scenarios, I realized that we need to keep the names for bincompat (because we permit old object files and static libraries, as long as the toolset used to perform the final link is newest). We could keep meow_clr for /clr and add meow_fwd for native but that would be verbose; I felt that just adding a comment explaining this was sufficient. This is a good lesson that we should try to name machinery based on what it does, not the specific circumstances that it happens to be used in today.

@StephanTLavavej StephanTLavavej added the build Related to the build system label Dec 7, 2021
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner December 7, 2021 05:18
stl/inc/mutex Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej added the affects redist Results in changes to separately compiled bits label Dec 15, 2021
@StephanTLavavej StephanTLavavej self-assigned this Dec 16, 2021
@StephanTLavavej

This comment has been minimized.

stl/src/xonce2.cpp Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej removed their assignment Dec 17, 2021
@StephanTLavavej
Copy link
Member Author

@CaseyCarter @cbezault I've verified that this now builds for arm64chk (which also builds arm64ec) and chpechk internally, so moving back to Ready To Merge. Please meow if you have any concerns.

@CaseyCarter
Copy link
Member

I'm going to add this to the next batch of changes to merge - please notify me if any further commits are pushed.

@CaseyCarter CaseyCarter merged commit dd5be88 into microsoft:main Jan 6, 2022
@CaseyCarter
Copy link
Member

Thanks for replacing the use of an unreleased internal tool in our build with the use of an undocumented linker feature! :string-of-cat-emojis:

I think this falls below the bar for Changelogging - "Replaced arcane details of obscure part of our build process with different but equally arcane details" just doesn't have that ring - but shout if you disagree and I'll try to come up with something reasonable.

@AlexGuteniev
Copy link
Contributor

It is visible:
[-] Optimization is now missing on clang and ARM64EC
[+] Optimization can be spread elsewhere, like to shared_mutex

@StephanTLavavej
Copy link
Member Author

Thanks @AlexGuteniev - as @CaseyCarter suggested, I've filed #2469 to track the future improvements you've noted. I think that the observable effects of this PR are small enough to fall below the Changelog threshold, but if you feel strongly about it we can write up an entry, just let us know. 😸

@walbourn
Copy link
Member

Note this change caused some breaking changes for static C++ libraries built with older versions of Visual Studio. See this issue.

@sylveon
Copy link
Contributor

sylveon commented May 19, 2022

#2655 gives a workaround

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects redist Results in changes to separately compiled bits build Related to the build system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants