@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.

Added LIT based tests.

I think this needs a re-review as I have deviated from the original simple approach.

Will this land soon? We are eagerly anticipating its arrival :)

Updated the diff following helpful review comments.

Add tests and prevent SemaTemplate and ItaniumCXXABI code paths from executing for PS4 (for now).

Addressed review comments.

Fixed indentation.

Hi @mstorsjo - I have been a bit short of time recently; but, I am going to look into everything that you have pointed out here. I should be able to make a reply by Monday.

In D90299#2362895, @rnk wrote:

I agree, the name could be improved. For prior art, see /Zc:dllexportInlines-, which disables some (but not all) of the dllexport/import semantics in clang-cl.

What we are really talking about here is what should be done when importing or exporting C++ constructs with vague linkage (inline functions, templates). That's what all the affected code has in common. Clang doesn't often use the terminology of vague linkage, so maybe it would be better to use the name comdat somehow.

How about TargetInfo::shouldDLLImportComdatSymbols? You could add in "export" (shouldDLLImportExportComdatSymbols), but GCC also exports comdat things sometimes, and in the cases where this predicate controls export behavior, it's pretty clear that it needs to be done to support the side which would import the symbol (explicit template instantiations).

Addressed the review comments and made some minor improvements.

addressed review comments and made some other improvements:

In D88877#2334222, @ikudrin wrote:

Hmm, that starts looking like a discussion for the mailing list, but I guess that all interested people are already here and the conversation will be better preserved for further references if being linked to the review.

In D88877#2332692, @bd1976llvm wrote:

Unfortunately, sometimes that is not that simple as it sounds. In our case, the users who need autolinking are different from a unit that provides tools to adjust command lines. There is no option to add anything at the very end.

Thanks for supplying this detail. To me this sounds like you need to file a bug against the tools unit? I want to evaluate your proposal on it's own merits rather accepting it to work around problems caused by other tools.

And for the tool unit folks that looks like an issue in the linker. Why should they work around problems caused by the linker? Why not fix them in the source? After all, build systems are many and reasoning for fixing them is vague. Closing "--whole-archive" range is more like tidiness, which was not that strictly required before (or without) the autolinking feature.

Apologies that I haven't had much time to look into this. I tried this style of patching with Sony's proprietary debugger and the user experience w.r.t. source locations and setting breakpoints was degraded. When I find time I will try to determine what is theoretically best here and whether the degradation in the debugging experience that I saw is an artefact of our proprietary debuggers implementation or if it points to something important. We should try with other consumers as well.

In D83069#2338104, @thopre wrote:

LGTM but I'm not familiar with lit to tell whether there would be a more idiomatic way to write this. I also agree with the assumption that invoking lit with a large number of tests arguments rather than a directory is unlikely.

Removed check for error message not being emitted.

In D88877#2331728, @ikudrin wrote:

In D88877#2331416, @bd1976llvm wrote:

Firstly, is this a real problem?

The problem is real. We have a report from our customer, so the issue is already observed in the wild.

In the RFC thread http://lists.llvm.org/pipermail/llvm-dev/2019-March/131105.html Rui said:

Then they can specify --no-whole-archive on the end of the command line, no?

Unfortunately, sometimes that is not that simple as it sounds. In our case, the users who need autolinking are different from a unit that provides tools to adjust command lines. There is no option to add anything at the very end.

In D88877#2316039, @ikudrin wrote:

• Simplified the comment.

Even if that was not decided in the discussion, I am pretty sure that --whole-archive cannot be used with dependent libraries, at least as the feature is implemented currently. The whole idea of the feature is that you can place #pragma comment(lib,...) in each and every source file which depends on the library. This results in multiple references to the library from many input object files, and if --whole-archive is active, this leads the linker to fail with the "duplicate symbol" error.

Improvements:

• Error rather than warn - lit tests are regression tests!
• Add option to supress check (there is no performance cost if this option is specified).
• Standardized on the terms "direct" and "indirect" in comments and diagnostics.
• Added missing test file.

I plan to investigate this further after the weekend; but, I wanted to say that the current scheme is known to be compatible with gdb and with Sony's proprietary debugger and yields a reasonable, if not ideal, debugging experience.

Use lld's mingw autolinking feature. Add a testcase that will fail without this.

In D88124#2294372, @mstorsjo wrote:

Hi mstorsjo, thanks for looking at this. Please add anyone else to this review that might be interested.

CMAKE improvements.

Removed the code patch and --patch functionality. These are not needed given that this review *is* a patch and having a patch in a patch is confusing.

ping!

In D85782#2273938, @hans wrote:

In D85782#2273402, @bd1976llvm wrote:

@hans, I think that we should put this change onto the release branch for llvm11.

I'm not familiar with the details of this patch, can you explain why we should merge it to llvm11?

We're very late in the release process, so unless it's fixing some major problem, I would be hesitant to merge.

@hans, I think that we should put this change onto the release branch for llvm11.

ping!

In D85782#2236493, @MaskRay wrote:

tl;dr This is a longstanding GNU ld bug introduced in binutils 2.26

% cat a.c
__attribute__((visibility("protected"))) void * foo (void) { return (void *)foo; }
% gcc -fpic a.c -shared -fuse-ld=bfd # relocation R_X86_64_PC32 against protected symbol `foo' can not be used when making a shared object

clang does not behave worse than GCC+GNU ld, so there is no regression on our side.

In D85782#2235567, @skan wrote:

I think this introduces an old issue when lowering the instruction.

ping!

In D85782#2211672, @bd1976llvm wrote:

In D85782#2211636, @MaskRay wrote:

GNU now recommends -fvisiblity=hidden (https://gcc.gnu.org/wiki/Visibility) so my understanding is that there will be very few STV_DEFAULT symbols.

I am not very sure this is now recommended but is indeed a non-rare configuration we should care about.

Agreed. Recommended is probably too strong.

Moved testing into linux-preemption.ll

In D85782#2211636, @MaskRay wrote:

The additional test file places too much burden on people maintaining the tests in the future. I think an additional @hidden_data = hidden global in CodeGen/X86/linux-preemption.ll should be sufficient.

In D73230#2208470, @MaskRay wrote:

The proposed canBenefitFromLocalAlias change looks good to me. Mind sending a patch?