In D133740#3787303, @MaskRay wrote:

In D133740#3786811, @weiwang wrote:

In D133740#3785549, @MaskRay wrote:

For Bazel like build systems, it seems weird to have one object file in two modes, lazy and non-lazy. Do you know why it happens for your build system?
I think the change will improve robustness of lld's distributed ThinLTO usage so we probably need to make the change.

We've seen the case of 2 same lazy objects in buck, and the cause for that is still a mystery to us. @christylee may know more.

Such a build system usually deduplicates linker input. I have seen duplicate archives but they are -lc, -lc++, etc, not from cc_library like targets.
Technically with linker scripts INPUT and GROUP an archive can be referenced by two cc_library, but that's a very rare case usually implying a user mistake but I feel the linker can guard against it, which will also help traditional build systems which don't do a great job deduplicating input.

The issue I'm facing is that if I use --whole-archive with --start-lib A.o --end-lib, the symbols that are included are not the same as those
if I were to just include libA.a. I would have expected that --start-lib A.o --end-lib would behave like archives, and that they would result
in the same symbol resolution.

In D120443#3341890, @MaskRay wrote:

I have thought reporting an error for such nested usage, but it needs some research that no build system relies on the behavior.
I'll be happy to check whether Bazel has nested usage. Perhaps you can do the same for your build system?

Updated to use the script parser so we can allow all KEEP semantics

In D85056#2188496, @MaskRay wrote:

--gc-sections throw away all unreferenced sections, but we sometimes need to keep some of them.

If an input section defines a non-local symbol, you can use -u to retain the section.

Although it's possible to use a linkerscript with the KEEP directive, it is often cumbersome for large repositories where each binary might have its own linkerscript.

Addressed comments, fixed unittest.

Moved test case to reproduce-lto.s

Removed debug comment.

BTW, do you have statistics about the effectiveness for this change?

In case we are concerned about correctness, we've been using this patch internally for a couple of months with no problems.

@grimar Do you know who might be a good reviewer?

Fixed comment.

We don't need to make the same change to COFF. COFF does not ignore comdats, it attempts to add a comdat symbol as long as the symbol is external. It also tracks whether a symbol is from a regular object file or bitcode file so it won't have duplicates.

Fixed a comment.

Changed variable name bitcodeComdatGroup to ltoOutputComdatGroup for consistency.

Addressed comments:

@sbc100 What would be a good way to go about writing a test for this? The reason why I noticed this bug was because .debug_types are not deduplicated in thinlto, but writing a lld test case using debug metadata seems clunky.

New diff: https://reviews.llvm.org/D80765

Fixed typo and removed unrelated change.

@hoy and I figured out the issue, the problem is caused by de-duplicating twice in thinlto, once in thin link, once in final link. Abandoning this in favor of a new approach. New diff coming up soon.

In D80255#2048298, @ruiu wrote:

How did it work with gold with the LTO plugin?

I tried building a more complex workload and the build failed due to some undefined hidden symbol. For thinlto, is the linker or codegen supposed to handle comdat folding?

Addressed comments. 1). made the same change to wasm. 2). removed the argument ignoreComdats.

However the behavior before and after this change should be the same, since I believe they were being ignored before this change too

We are trying to migrate from gold to lld, and we found that when building with thinlto, lld does not de-duplicate .debug_types. De-duplication is successful with monolithic lto. Repro: clang -flto=thin -fdebug-types-section -fuse-ld=lld -Wl,-plugin-opt=-generate-type-units

include/llvm/ADT/SmallVector.h:153 has an assert statement, but I'm not sure why I'm the only one who's experienced problems so far.

Thanks for catching that!

Could you elaborate a bit on why hot text (is that .text.hot section?) needs to have a stricter alignment requirement than the common-page-size?

Do you know why that system uses not only max-page-size but also common-page-size?

I wonder if you found a use case of -z common-page-size with a page size that is different from the ABI's default. For example, did you find a use case of the option for x86-64 with a page size larger than 4096?

I spoke with @modocache and I'm going to take a crack at this.

In D53536#1277610, @clin1 wrote:

One more favor to ask: Could I please have some help committing this fix? I'm new to the project and don't have write access yet...

The changes were landed in D52433

@courbet , @lebedev.ri , I'm trying to commit the patch by first running "arc patch D51550", but it keeps saying unable to apply patch because llvm/trunk/test/Transforms/MergeICmps/X86/alias-merge-blocks.ll does not exists in index. How should I go about this?

Going back to previous revision to include the test in this revision.

Thanks for the fix!

Added tests.

@rsmith , thanks for the review, I fixed the variable capitalization. If you could land it for me that'll be awesome!

Fixed diff dependencies.

Addressed @lebedev.ri 's comment and added dependent child diff to show original check-lines.

Addressed @rsmith 's comments.

Emit non-fatal error for typo if file exists.

And of course thank you to @trentxintong :)

Thanks for the review! @efriedma

Addressed @courbet comment

Removed clang-format changes unrelated to this patch.

For instructions that may have side effects, check that the instruction is a simple load or simple store before checking for alias. For things like function calls, we would have to check that they don't throw and have no read/write to memory. The function calls are potentially nested as well, it's probably better to just bail on those cases.

Friendly ping @courbet :)