This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] [test] Use -fms-runtime-lib= for picking the CRT to use
ClosedPublic

Authored by mstorsjo on Jul 18 2023, 1:06 AM.

Details

Summary

This is a more correct way of linking against a specific runtime
library with the GCC-like clang frontend.

This avoids having to pass -D_DEBUG (and passes flags like -D_DLL,
which we should be passing if linking against the dynamic CRT).

When -fms-runtime-lib= is specified, each compiled object file gets
embedded directives instructing the linker about what CRT it should
link against. The -nostdlib we pass to the compiler driver only
inhibits the libs that the compiler driver passes to the linker
command.

Thus, this also avoids having to specify -lmsvcrt, -lmsvcrtd,
-llibcmt or -llibcmtd and -loldnames.

Based on a patch by Andrew Ng.

The -fms-runtime-lib= option was added in Clang 16, so this patch
can only be landed once libcxx's compiler requirement has been
bumped to Clang 16 (which is expected to be done sometime after
Clang 17 has been branched or released).

Diff Detail

Event Timeline

mstorsjo created this revision.Jul 18 2023, 1:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 18 2023, 1:06 AM
mstorsjo requested review of this revision.Jul 18 2023, 1:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 18 2023, 1:07 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

This LGTM but perhaps this should be rolled in with or done after the de-duplication of these test config files?

This LGTM but perhaps this should be rolled in with or done after the de-duplication of these test config files?

Dunno - I'm keeping it separate at least, to allow a separate decision on whether such a deduplication (moving more config-specific logic from the test configs into cmake) is a good or bad thing.

After rebasing, there were some unrelated test failures in the module build here.

mstorsjo updated this revision to Diff 546393.Aug 2 2023, 3:42 AM

Rebased, with the deduplication done before this.

Mordante accepted this revision.Aug 19 2023, 4:08 AM

LGTM but please wait for clang-16 to be the minimal supported Clang version and wait for the approval of @andrewng

This revision is now accepted and ready to land.Aug 19 2023, 4:08 AM
ldionne accepted this revision.Sep 14 2023, 10:37 AM
ldionne added a subscriber: ldionne.

[Github PR transition cleanup]

This patch makes sense to me. I am dropping Clang 15 here: https://github.com/llvm/llvm-project/pull/66406.

Can you please rebase to poke CI and ship this once the above PR has been merged? I'll make a note to ping here when I merge it.

andrewng accepted this revision.Sep 15 2023, 2:44 AM

@mstorsjo Can you rebase this now? I just dropped support for Clang 15.

mstorsjo updated this revision to Diff 557375.Sep 26 2023, 1:45 PM

Rebased, rerunning CI, will land if things look fine.

Rebased, rerunning CI, will land if things look fine.

The CI mostly completed; the benchmarks build seems to have failed due to something unrelated, and a few macOS jobs are still waiting 14h later; I'll go ahead and land this.

This revision was landed with ongoing or failed builds.Sep 27 2023, 3:53 AM
This revision was automatically updated to reflect the committed changes.