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. Addresed reivew 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 :)

Fix off-by-one problem.

Fix linux build and formatting issues.

Thanks for the timely fix :)

Remove debugging tee.

Arrg sorry! Uploaded an earlier incomplete revision by mistake.

In D107216#2918203, @MaskRay wrote:

I think @bd1976llvm have found a way forward.

In D100944#2870297, @MaskRay wrote:

Augment compiler section attribute to allow specifying the section flags as well as the section name.

Recent GNU as reports errors for changed section attributes, so gcc with such a GNU as will report an error

% cat a.c
asm(".section debug_special,\"\",@progbits");
__attribute__((used, section("debug_special"))) int var;

% gcc -c a.c
/tmp/ccrwXjUO.s: Assembler messages:
/tmp/ccrwXjUO.s:7: Error: changed section attributes for debug_special

So I think a compiler oriented solution is more appropriate than a linker oriented one.
A GNU attribute beside section looks good to me. Such a request needed to be raised on the GCC Bugzilla.

Thanks for replying with some potential solutions. I'm sorry for not raising this earlier we have been slow to keep up with upstream changes recently so this change didn't get merged and tested until last week.

We have found a problem whist testing this change. We have some internal code that was exploiting the old behaviour:

In D100944#2770791, @peter.smith wrote:

Thanks very much for working on this. I agree with the approach of putting out a separate section when the flags don't match as this matches the non-lto behaviour when the files are compiled separately.

@tstellar sorry to be a pain but could you give a go or no-go for putting this on the release branch please? For internal reasons I have to finish LLVM 12 changes this week. Thanks!

In D101890#2741842, @tstellar wrote:

Can you file a bug for this?

@tstellar this is a very safe change and we did have a customer confused by this option. Would it be possible to put this change onto the 12 release branch? Thanks.

Conform to mandoc style.

Thanks @peter.smith. I have added the man page update.

Hi. Can you add some background information to the description. I assume the discussion on https://reviews.llvm.org/D93948 is relevant. Is there an associated bug?

@compnerd, @smeenai do you have any further comments?

ping.

In D89518#2598986, @smeenai wrote:

Sorry for the delay here. This looks good; thank you!

ping :)

ping!

@mstorsjo are you happy with this now?

Added the cmake defines from https://reviews.llvm.org/D90021 for the libcxxabi build.

In D83069#2496184, @thopre wrote:

Hi @bd1976llvm

I've just realized in many projects I'm involved in testsuites are managed by cmake and lit is only ever invoked on single testcase. Without passing --no-indirectly-run-check tests fail but when using an old lit I get an error if using this option. Can we allow to turn off the feature via environment variable or configuration?

Thanks in advance.

I have added this patch to https://reviews.llvm.org/D88124 along with some end to end tests to show the scei vendor windows-itanium behaviour. From the 'wi-test/test/dll_vtable_missing/test.py' test you can see that the link error given by the scei vendor variant is more consistent than the standard windows-itanium. Both variants should be equivalent in terms of runtime behaviour.

Added the patch from https://reviews.llvm.org/D93203 and the scei vendor variant of windows itanium to the relevant tests.

Thanks for the suggestions. I have reworded the comments so that they are hopefully much clearer.

rebased build script and some minor improvements.

Thanks for your comments. I tried to do a rebase right up to the head of main - but it proved very difficult to resolve all the problems this caused. Instead I have rebased to your https://github.com/llvm/llvm-project/commit/8a73aa8c4c3ee82e7d650c6a957937713a4a9ab4

@mstorsjo I'm looking at this again after the winter break. Do you have any further comments?

Removed COMDAT limitation text.

@compnerd @smeenai are you basically happy with this doc now?

Addressed review comments.

Added type_info tests.

Updated comment to state that "semantic compatibility" is the goal.

Thanks for the comments. I have updated the diff.

Fixed spelling errors.

Improved comment.

In D91779#2405841, @compnerd wrote:

Im not sure if renaming the check prefix is actually helpful, but sure. This does seem like somewhat unfortunate. For the dllexport case, you really want dso_local. define dso_local global and declare dso_local global are preferable in this case as that guarantees a local definition, which is always the case for an exported symbol. You definitely do not want declare extern dso_local global. A few of the changes here are actually changing the IRGen from the desired state, is that intentional?

Let me know if you think that more tests need adding. I have added support for LIT based run tests to the build recipe (https://reviews.llvm.org/D88124) I can use this to add any run tests that might be useful for this review.