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

[LLD] [COFF] Add a separate option for allowing duplicate weak symbols #68077

Merged
merged 1 commit into from
Oct 20, 2023

Conversation

mstorsjo
Copy link
Member

@mstorsjo mstorsjo commented Oct 3, 2023

The MinGW mode (enabled with the flag -lldmingw) does allow duplicate weak symbols. A test in
compiler-rt/test/profile/Windows/coverage-weak-lld.cpp does currently enable the -lldmingw flag in an MSVC context, in order to deal with duplicate weak symbols.

Add a new, separate, lld specific flag for enabling this. In MinGW mode, this is enabled by default, otherwise it is disabled.

This allows making the MinGW mode more restrictive in adding libpaths from the surrounding environment; in MinGW mode, all libpaths are passed explicitly by the compiler driver to the linker, which is attempted in https://reviews.llvm.org/D144084.

The MinGW mode (enabled with the flag -lldmingw) does allow duplicate
weak symbols. A test in
compiler-rt/test/profile/Windows/coverage-weak-lld.cpp does currently
enable the -lldmingw flag in an MSVC context, in order to deal with
duplicate weak symbols.

Add a new, separate, lld specific flag for enabling this. In MinGW
mode, this is enabled by default, otherwise it is disabled.

This allows making the MinGW mode more restrictive in adding
libpaths from the surrounding environment; in MinGW mode, all
libpaths are passed explicitly by the compiler driver to the
linker, which is attempted in https://reviews.llvm.org/D144084.
@mstorsjo
Copy link
Member Author

@MaskRay This relates to the use of -lldmingw in compiler-rt/test/profile/Windows/coverage-weak-lld.cpp (in MSVC mode); I’d like to add this new flag and move that test over to using that flag. Because I would like to change -lldmingw so that it doesn’t add MSVC lib directories automatically, see https://reviews.llvm.org/D144084. Do you know if that flag is used in that way in MSVC environments anywhere else outside of that one test?

@MaskRay
Copy link
Member

MaskRay commented Oct 19, 2023

A test in compiler-rt/test/profile/Windows/coverage-weak-lld.cpp does currently enable the -lldmingw flag in an MSVC context, in order to deal with duplicate weak symbols.

I added the test in in 2021 when investigating Windows's weak symbols: https://maskray.me/blog/2021-04-25-weak-symbol#pecoff

I haven't added similar tests elsewhere. Thank you for making a standalone option. I wonder whether anyone has a channel to tell MSVC that this is a desired feature that should be enabled by default.

RUN: lld-link -lld-allow-duplicate-weak %S/Inputs/gnu-weak.o %S/Inputs/gnu-weak2.o -out:%t.exe
RUN: not lld-link %S/Inputs/gnu-weak.o %S/Inputs/gnu-weak2.o -out:%t.exe 2>&1 | FileCheck %s --check-prefix=DEFAULT-ERROR

DEFAULT-ERROR: error: duplicate symbol: weakfunc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is the last line, consider adding DEFAULT-ERROR-NOT: {{.}} as a change detector when the diagnostic improves.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are more lines following this diagnostic (defined at /path/to/object/file), so I'll leave this as is.

@mstorsjo mstorsjo merged commit a67ae8c into llvm:main Oct 20, 2023
3 checks passed
@mstorsjo mstorsjo deleted the lld-allow-duplicate-weak branch October 20, 2023 20:44
@mstorsjo
Copy link
Member Author

I added the test in in 2021 when investigating Windows's weak symbols: https://maskray.me/blog/2021-04-25-weak-symbol#pecoff

I haven't added similar tests elsewhere.

Ok, thanks for the info!

I wonder whether anyone has a channel to tell MSVC that this is a desired feature that should be enabled by default.

I guess one could post a feature request at https://developercommunity.visualstudio.com/cpp; I'm not very hopeful that it'd be implemented, but it would at least be the official way of signaling the request.

mstorsjo added a commit that referenced this pull request Oct 21, 2023
In mingw mode, all linker paths are passed explicitly to the linker
by the compiler driver. Don't try to implicitly add linker paths
from the LIB environment variable or by detecting an MSVC
installation directory.

If the /winsysroot command line parameter is explicitly passed to
lld-link while /lldmingw is specified, it could be considered reasonable
to actually include those paths. However, modifying the code to
handle only the /winsysroot case but not the other ones, when the
mingw mode has been enabled, seems like much more code complexity
for a mostly hypothetical case.

Add a test for this when case when using LIB. (The code paths for
trying to detect an MSVC installation aren't really regression tested.)

Also fix an issue in the existing test for "Check that when /winsysroot
is specified, %LIB% is ignored.", where the LIB variable pointed
to a nonexistent directory, so the test would pass even if /winsysroot
wouldn't be specified.

Reland this after #68077 and
#69781 - the compiler-rt test
that used -lldmingw in MSVC environments has been updated to use a more
specific option.

Differential Revision: https://reviews.llvm.org/D144084
hamishknight pushed a commit to hamishknight/llvm-project that referenced this pull request Jul 2, 2024
llvm#68077)

The MinGW mode (enabled with the flag -lldmingw) does allow duplicate
weak symbols. A test in
compiler-rt/test/profile/Windows/coverage-weak-lld.cpp does currently
enable the -lldmingw flag in an MSVC context, in order to deal with
duplicate weak symbols.

Add a new, separate, lld specific flag for enabling this. In MinGW mode,
this is enabled by default, otherwise it is disabled.

This allows making the MinGW mode more restrictive in adding libpaths
from the surrounding environment; in MinGW mode, all libpaths are passed
explicitly by the compiler driver to the linker, which is attempted in
https://reviews.llvm.org/D144084.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants