In D138463#3944124, @compnerd wrote:

The change itself seems fine, but the CI failure seems related to this change.

Fix for test failure on *nix.

Set DLL storage class for guard variables correctly to avoid a link error when code using a guard variable is inlined into a module that imports the code.

Addressed review comments.

Abandoned in favour of: https://reviews.llvm.org/D135897.

In D135737#3854200, @MaskRay wrote:

(--wrap is so complex that I can't come up with a list of rules to follow. I can only make sense by inspecting test output difference.)

For wrap-extract-real.s, I don't think the new behavior improves semantics.

ld.lld %t/a.o --wrap foo  # error: undefined symbol: __real_foo
ld.lld %t/a.o --start-lib %t/b.o --end-lib --wrap foo  # ok, even if %t/b.o is not extracted. This is weird

Generally, if an archive member is not extracted, we expect that there is no symbol resolution difference.

Improve testing and add tests to show that symbol foo is considered referenced by LLD even if all references are redirected by --wrap foo. This property means that a __wrap_foo function that contains calls to __real_foo will not be called if foo is not included in the link.

Address final review comments.

In D134784#3822333, @MaskRay wrote:

In D134784#3822303, @bd1976llvm wrote:

In D134784#3821336, @MaskRay wrote:

For Clang, it seems that MSVC allows dllspec on a local linkage variable, so that's why we have to add it to CodeGen instead of rejecting it in Sema.

Can you give an example? MSVC certainly rejects simple cases e.g. https://godbolt.org/z/bG3M7oj5W. I am worried that I have missed something...

__declspec(dllexport) const int foo = 10; foo with internal linkage is accepted.

In D134784#3821336, @MaskRay wrote:

For Clang, it seems that MSVC allows dllspec on a local linkage variable, so that's why we have to add it to CodeGen instead of rejecting it in Sema.

Thanks for the review. I addressed your comments and also consolidated the tests into a single test using split-file.

Whilst reviewing the impact of these dllimport/export restrictions I noticed that LLVM IR does not reject dllimport/export on local linkage globals. I have put up a PR for adding that restriction: D134784 (https://reviews.llvm.org/D134784).

Thanks. LGTM.

I reverted this change as it breaks the tests on windows. It seems that distutils.spawn.find_executable is not exactly equivalent to shutil.which on Windows as the former adds ".exe" and the latter adds the extensions from PATHEXT. Furthermore we are invoking these functions on a full path e.g. "C:\p\15\vs2019\bin\clang" and looking at the docs I'm not sure what the behaviour of either function should be in that case.

In D133266#3776164, @MaskRay wrote:

In D133266#3776051, @bd1976llvm wrote:

In D133266#3775832, @MaskRay wrote:

In D133266#3775384, @MaskRay wrote:

In D133266#3775189, @bd1976llvm wrote:

From a glance, -fvisibility-global-new-delete-hidden should make the visibility implicit (so won't trigger this error) but don't seem to do so? I'll investigate later.

-fvisibility-global-new-delete-hidden is implemented by adding VisibilityAttr to declarations.
I think -fvisibility-global-new-delete-hidden triggering the error is fine. The alternative, adding a rule that __declspec(dllexport) void operator delete does not get hidden visibility, seems ad-hoc to me.

I'm not sure why this visibility option (-fvisibility-global-new-delete-hidden) is implemented differently to the others (e.g. -fvisibility=hidden)? __declspec(dllexport) void operator delete does not get hidden visibility, might be difficult to implement; but OTOH, the dllstorageclass overrides the visibility set by a visibility option for the other visibility options (e.g. -fvisibility=hidden) and it would be nice to have consistent behaviour for all the visibility options. Would be great to get other peoples opinions on whether an error here would be a problem?

First, does COFF use -fvisibility-global-new-delete-hidden? The impl of -fvisibility-global-new-delete-hidden is very different from -fvisibility. -fvisibility changed visibility is considered !isVisibilityExplicit while -fvisibility-global-new-delete-hidden's is isVisibilityExplicit.

dllspec and visibility are extensions for different object file formats and here we are mixing them together.
Personally I don't favor defining too many "let A override B" designs. Adding if (GV->hasDLLExportStorageClass() || GV->hasDLLImportStorageClass()) is mostly only due to compatibility consideration...

In D133266#3775832, @MaskRay wrote:

In D133266#3775384, @MaskRay wrote:

In D133266#3775189, @bd1976llvm wrote:

From a glance, -fvisibility-global-new-delete-hidden should make the visibility implicit (so won't trigger this error) but don't seem to do so? I'll investigate later.

-fvisibility-global-new-delete-hidden is implemented by adding VisibilityAttr to declarations.
I think -fvisibility-global-new-delete-hidden triggering the error is fine. The alternative, adding a rule that __declspec(dllexport) void operator delete does not get hidden visibility, seems ad-hoc to me.

This approach doesn't account for the implementation of -fvisibility-global-new-delete-hidden. The following case now fails after this change:

In D133267#3773235, @MaskRay wrote:

In D133267#3771894, @bd1976llvm wrote:

Hi! Sorry for not raising this issue prior to commit. The dllimport restriction seems correct to us. However, in PlayStation we are using dllexportclass=dllexport visibility=protected GlobalValues in LLVM IR. The protected visibility indicates that the symbol cannot be preempted and could be exported and dllexport indicates that it should be exported. This allows for implementing export control from C/C++ source code. This change has made this usage illegal. It doesn't makes sense semantically to restrict dllexport to visibility=default in LLVM IR, the restriction should be to visibility != hidden as hidden symbols cannot be exported by ELF linkers. Thanks.

Oh, didn't know. Pushed 97d00b72a2b0a7aca631e1402a647f32c4e8bafb to allow dllexport protected.
Does PlayStation need to relax the clang check D133266, too? (I have noticed that this combo makes sense but rejected it for simplicity.)

Hi! Sorry for not raising this issue prior to commit. The dllimport restriction seems correct to us. However, in PlayStation we are using dllexportclass=dllexport visibility=protected GlobalValues in LLVM IR. The protected visibility indicates that the symbol cannot be preempted and could be exported and dllexport indicates that it should be exported. This allows for implementing export control from C/C++ source code. This change has made this usage illegal. It doesn't makes sense semantically to restrict dllexport to visibility=default in LLVM IR, the restriction should be to visibility != hidden as hidden symbols cannot be exported by ELF linkers. Thanks.

Committed in: https://github.com/llvm/llvm-project/commit/325e7e8b87423f83ba9fc53b9486ce67329d486c.

In D128452#3623184, @MaskRay wrote:

at link time is incorrect for distributed ThinLTO which happens at clang -fthinlto-index=... time. You may just say "ThinLTO backend compilation phase"

Addressed review comments.

Added some reviewer from D112160 where PGL has recently been made to work for macho LLD. It seems likely (to me!) that macho is also affected by this issue.

In D128452#3607209, @nikic wrote:

(Not familiar with CGProfilePass and its requirements)

• Removed the LLD_IN_TEST un-setting in the lit test as we have moved the code earlier now.
• Removed checking for the period/full-stop in the lit test to allow customizers to extend the sentence without adjusting this test.

Ensure added lit test passes for LLVM_ENABLE_BACKTRACES=off builds.
Added differential URL to release note.
Used regex so that test doesn't care about contents of the bug report URL.

Move to lld.cpp

Remove accidentally committed comments.

Fix typo.

Addressed style comments.

Add ReleaseNote.

In D128195#3596905, @tschuett wrote:

Entry in ReleaseNotes or man page?

In D128195#3596592, @peter.smith wrote:

Alternatively would a --crash option be easier to manage? I don't think we can replicate the reproducer that the clang driver uses with this option.

Added useful reminder message as suggested by Peter.

I have made a commit with the current improvements.

In D127915#3590333, @dblaikie wrote:

how's this compare to the linux behavior? be nice to have a similar format

I have asked around and no one made use of those numbers so I have removed them in the latest diff. However, people would have found the exception code useful so I have added that. Output now looks like:

Sorry for the delay. I wanted to wait until https://github.com/llvm/llvm-project/issues/55527 was fully understood in case we needed to adjust the test or add extra cases.

update diff to a working regression test!

@DiggerLin there are a number of tests e.g: llvm\test\tools\llvm-ar\regular-to-thin-archive.test that are still marked "XFAIL: system-aix" that probably need looking at. It might be good to write down what testing strategy you want for AIX. Are you happy to rely on --format=gnu test coverage for most of the tests and have a few specifically --format=aix tests?

@MaskRay - thanks for the review comments, I will keep your advise w.r.t. urls in commit comments in mind.

Hi @DiggerLin. Currently the --help text states --format=aix but llvm-ar accepts --format=bigarchive. Could you fix that please.

In D125439#3517356, @nkhoun wrote:

@bd1976llvm do you have an ETA to address the AIX failure Jake mentions above?

Address final comments.

Now using split-file and cd. Removed an invalid test-case. Addressed review comments.

Improved test and addressed review comments.

I am slightly concerned about penalising links with low utilisation archives. However, I don't have any specific data on low utilisation archives or any example of them. We did do some testing of a similar feature (in our proprietary linker) on game code, and low utilisation archives did not cause a problem for any of the games tested. Users should be able to create high utilisation versions of problematic archives in any case.

Committed in https://github.com/llvm/llvm-project/commit/666aa43cbf829421b470c1ef6cf585d1dd3c94fd.

In D118577#3288196, @MaskRay wrote:

This .strtab deduplication makes parallel .symtab write difficult. I suspect we may need to disable it entirely in the future.

In D117933#3267645, @MaskRay wrote:

In D117933#3267519, @mcgrathr wrote:

In D117933#3267443, @MaskRay wrote:

Currently a deplib name (${name}) indicates either the literal ${name} or the -l style lib${name}.{a,so}.
We have a conflict if ${name} is foo and there is a directory named foo.

What if we do

if (name.endswith(".a") || name.endswith(".so"))
  findFromSearchPaths
else
  searchLibraryBaseName

?

That should avoid the conflict, too. A directory is unlikely named .so or .a.

I don't think introducing more magical rules about naming patterns ever makes things easier.

It is surprising at first blush that deplibs searches for "foo" and not just what "-lfoo" would do, but it makes sense if you want to allow saying "libfoo.a" in deplibs to avoid matching "libfoo.so" in arcane cases.
So I think just the fix here to make all cases of not-existing-files candidates on search paths be ignored robustly is sufficient. The extra set of candidates that deplibs cases will attempt is tolerable and IMHO it's far preferable to having any more magic in the semantics of the behavior than there already is

It's not adding more magical rules. The chained findFromSearchPaths and searchLibraryBaseName is replaced with testing just one form.
We can then get rid of llvm::function_ref<bool(StringRef)>.

In D117284#3254734, @peter.smith wrote:

No objections. Reading your blog post and my copy of Linkers and Loaders it does seem that early linkers would handle archives with indexes, it just seems like indexes were ubiquitous when GNU ld were written. So it does seem like rejecting archives without an index is a property of the implentation of GNU ld rather than something that should be relied upon.

In D117284#3243370, @sbc100 wrote:

In D117284#3243218, @bd1976llvm wrote:

Another advantage is that this behaviour allows users to "repair" archives with incomplete symbol tables by stripping the symbol table (maybe someone has added both ET_RELs and bitcode objects to the same archive with an ar which ignored the bitcocde symbols).

Isn't it already trivial to "repair" such libraries by running ranlib on them?

Another advantage is that this behaviour allows users to "repair" archives with incomplete symbol tables by stripping the symbol table (maybe someone has added both ET_RELs and bitcode objects to the same archive with an ar which ignored the bitcocde symbols).

Sorry I missed this review - looks great! Did you try getting performance numbers using the tar packages that Rafael gathered up?

In D110716#3047094, @MaskRay wrote:

Sorry for the slow response. I was out of town for one week.

Do not test reloc offset as its value isn't important.

Additionally augmented weak-undef.s and weak-undef-rw.s with a --hex-dump test.

Thanks for working on this. In the past I have resorted to hacking in my own ad-hoc versions of such features to solve particularly horrible dependency cases. As mentioned above there's a case for more powerful dependency tracking e.g. https://reviews.llvm.org/D69607. The more of these debugging features available the better IMO :)