[LTO] Make local linkage GlobalValue in non-prevailing COMDAT available_externally

[MaskRay]

Summary

For a local linkage GlobalObject in a non-prevailing COMDAT, it remains defined while its
leader has been made available_externally. This violates the COMDAT rule that
its members must be retained or discarded as a unit.

To fix this, update the regular LTO change D34803 to track local linkage
GlobalValues, and port the code to ThinLTO (GlobalAliases are not handled.)

This fixes two problems.

(a) __cxx_global_var_init in a non-prevailing COMDAT group used to
linger around (unreferenced, hence benign), and is now correctly discarded.

int foo();
inline int v = foo();

(b) Fix https://github.com/llvm/llvm-project/issues/58215:
as a size optimization, we place private __profd_ in a COMDAT with a
__profc_ key. When FuncImport.cpp makes __profc_ available_externally due to
a non-prevailing COMDAT, __profd_ incorrectly remains private. This change
makes the __profd_ available_externally.

cat > c.h <<'eof'
extern void bar();
inline __attribute__((noinline)) void foo() {}
eof
cat > m1.cc <<'eof'
#include "c.h"
int main() {
  bar();
  foo();
}
eof
cat > m2.cc <<'eof'
#include "c.h"
__attribute__((noinline)) void bar() {
  foo();
}
eof

clang -O2 -fprofile-generate=./t m1.cc m2.cc -flto -fuse-ld=lld -o t_gen
rm -fr t && ./t_gen && llvm-profdata show -function=foo t/default_*.profraw

clang -O2 -fprofile-generate=./t m1.cc m2.cc -flto=thin -fuse-ld=lld -o t_gen
rm -fr t && ./t_gen && llvm-profdata show -function=foo t/default_*.profraw

If a GlobalAlias references a GlobalValue which is just changed to
available_externally, change the GlobalAlias as well (e.g. C5/D5 comdats due to
cc1 -mconstructor-aliases). The GlobalAlias may be referenced by other
available_externally functions, so it cannot easily be removed.

Depends on D137441: we use available_externally to mark a GlobalAlias in a
non-prevailing COMDAT, similar to how we handle GlobalVariable/Function.
GlobalAlias may refer to a ConstantExpr, not changing GlobalAlias to
GlobalVariable gives flexibility for future extensions (the use case is niche.
For simplicity we don't handle it yet). In addition, available_externally
GlobalAlias is the most straightforward implementation and retains the aliasee
information to help optimizers.

See windows-vftable.ll: Windows vftable uses an alias pointing to a
private constant where the alias is the COMDAT leader. The COMDAT use case
is skeptical and ThinLTO does not discard the alias in the non-prevailing COMDAT.
This patch retains the behavior.

[tejohnson]

Regular LTO is not fixed yet.

Looks like handleNonPrevailingComdat in LTO.cpp would need a similar fix.

When FuncImport.cpp ...

Note that while this functionality currently lives in FunctionImport.cpp it isn't really related to function importing, rather to ThinLTO symbol resolution. So mentioning the file name might be a little misleading (but if you do, correct it to FunctionImport.cpp).

@xur can you confirm this fixes the original problem?

llvm/test/ThinLTO/X86/linkonce_resolution_comdat.ll

22

I assume the rest of the cases above (i.e. besides g_internal) already worked before this patch, including g_private since it is not local (see question about this below), is that correct?

44

Was this meant to have private linkage?

[xur]

In D135427#3842972, @tejohnson wrote:

Regular LTO is not fixed yet.

Looks like handleNonPrevailingComdat in LTO.cpp would need a similar fix.

When FuncImport.cpp ...

Note that while this functionality currently lives in FunctionImport.cpp it isn't really related to function importing, rather to ThinLTO symbol resolution. So mentioning the file name might be a little misleading (but if you do, correct it to FunctionImport.cpp).

@xur can you confirm this fixes the original problem?

Yes. This fix also works. This basically retains the COMDAT group for the profile variables in LTO builds -- the same as non-LTO builds.

[rnk]

This makes sense. I think there may be an analogous situation for ASan and inline globals with dynamic initialization on COFF, see the comdat group usage here:
https://gcc.godbolt.org/z/3rMo3M6cv

If one were to run ThinLTO on that IR, you'd end up with double-registered ASan globals.

[MaskRay]

Fix regular LTO as well

[tejohnson]

Thanks for fixing regular LTO as well. Question on test change for that, and I had a couple questions on the ThinLTO test.

llvm/lib/LTO/LTO.cpp

698–699

Update comment.

llvm/test/LTO/Resolution/X86/comdat-mixed-lto.ll

18–19

Any idea where this is getting dropped to a declaration before optimization?

[MaskRay]

update comment

[MaskRay]

llvm/test/LTO/Resolution/X86/comdat-mixed-lto.ll

18–19

I am very familiar with IRMover but the logic is in

LTO.cpp RegularLTO.Mover->move
...
IRMover.cpp:594 Expected<Constant *> NewProto = linkGlobalValueProto(SGV, ForIndirectSymbol);
IRMover.cpp:1038 NewGV = copyGlobalValueProto(SGV, ShouldLink || ForIndirectSymbol);

shouldLink returns false for the available_externally GlobalObject.

Then D28737 ensures the global_ctors entry is dropped (since the comdat object is available_externally).

llvm/test/ThinLTO/X86/linkonce_resolution_comdat.ll

22

Objects except g_internal/g_private work before this patch. g_internal/g_private work with this patch.

44

g (comdat key) is meant to have private linkage. A comdat with a local linkage key should not be used for portability.

[tejohnson]

llvm/lib/Transforms/IPO/FunctionImport.cpp

1148

maybe add assert below that anything we find has local linkage.

llvm/test/ThinLTO/X86/linkonce_resolution_comdat.ll

44

Sorry, not following. How/when does g_private get private linkage? Because it is external here afaict.

[MaskRay]

Make g_private in tests private.
Add an assert

llvm/test/ThinLTO/X86/linkonce_resolution_comdat.ll

44

Sorry. Missed private for g_private (the suffix is to indicate the original linkage). Added

[tejohnson]

lgtm

[MaskRay]

llvm/test/LTO/Resolution/X86/comdat-mixed-lto.ll

11

The test has an issue. If C is prevailing in %t1.o, its member testglobfunc must be prevailing as well.

I'll swap the two -r options for testglobfunc: -r=%t1.o,testglobfunc,lxp -r=%t2.o,testglobfunc,lx

[tejohnson]

llvm/test/LTO/Resolution/X86/comdat-mixed-lto.ll

11

This was by design. It was trying to simulate an issue that occurred with mixed Thin and regular LTO modules (per some discussion on original patch and bug). I'm not sure if we need to keep it that way, but I noticed you removed the check of testglobfunc being made available_externally - what happened to that check?

[MaskRay]

llvm/test/LTO/Resolution/X86/comdat-mixed-lto.ll

11

assert(GO.hasLocalLinkage() && "GO's comdat should have been dropped"); fails with -r=%t2.o,testglobfunc,lxp -r=%t1.o,testglobfunc,lx.

Restore the original RUN line and remove the assert then? Note that the assert can fire in other erroneous cases where a user uses an external linkage definition to prevail a linkonce_odr definition in a prevailing comdat.

[tejohnson]

llvm/test/LTO/Resolution/X86/comdat-mixed-lto.ll

11

Note that the assert can fire in other erroneous cases where a user uses an external linkage definition to prevail a linkonce_odr definition in a prevailing comdat.

Is this a case we want to detect and assert on in this code? Should it be detected and flagged elsewhere if it is erroneous?

[MaskRay]

Rebase after addrsig fix
Update description that this fixes a __cxx_global_var_init problem

[MaskRay]

I put this in the description

To fix this, update the regular LTO change D34803 to track local linkage GlobalValues, and port the code to ThinLTO (GlobalAliases are not handled.)

It turns out that we need to handle GlobalAlias in this patch (as opposed to in a follow-up), as otherwise it may break clang codegen of C5/D5 constructor/destructor

% cat a.ll
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

$_ZN1AD5Ev = comdat any
$_ZTV1A = comdat any

@_ZN1AD1Ev = weak_odr unnamed_addr alias void (ptr), ptr @_ZN1AD2Ev
; @_ZTV1A = weak_odr unnamed_addr constant { [4 x ptr] } { [4 x ptr] [ptr null, ptr @_ZTI1A, ptr @_ZN1AD1Ev, ptr @_ZN1AD0Ev] }, comdat, align 8
@_ZTV1A = weak_odr unnamed_addr constant { [4 x ptr] } { [4 x ptr] [ptr null, ptr null, ptr @_ZN1AD1Ev, ptr @_ZN1AD0Ev] }, comdat, align 8

define weak_odr void @_ZN1AD2Ev(ptr noundef nonnull align 8 dereferenceable(464) %0) unnamed_addr #9 comdat($_ZN1AD5Ev) align 32 {
  ret void
}

define weak_odr void @_ZN1AD0Ev(ptr noundef nonnull align 8 dereferenceable(464) %0) unnamed_addr #9 comdat($_ZN1AD5Ev) align 32 {
  tail call void @_ZN1AD1Ev(ptr noundef nonnull align 8 dereferenceable(464) %0)
  ret void
}

% opt -module-sumary a.ll -o a.bc && cp a.bc b.bc
% ld.lld a.bc b.bc
...verifier error

The IR above is reduced from C++ examples:

// a.cc
template <typename T>
struct A {
  A() {}
  virtual ~A() {}
};

void *fa() { return new A<void>; }

// b.cc
template <typename T>
struct A {
  A() {}
  virtual ~A() {}
};

template struct A<void>;

void *fb() { return new A<void>; }

% clang -flto=thin -c a.cc b.cc -fpic
% ld.lld a.o b.o -shared   # if GlobalAlias (e.g. ` @_ZN1AIvED1Ev = weak_odr unnamed_addr alias void (ptr), ptr @_ZN1AIvED2Ev`) is not handled
Alias must point to a definition                                                                                                                                                                                                               
ptr @_ZN1AIvED1Ev                                                                                                                                                                                                                              
LLVM ERROR: Broken module found, compilation aborted! 
...

[MaskRay]

handle GlobalAlias

[tejohnson]

In D135427#3869003, @MaskRay wrote:

I put this in the description

To fix this, update the regular LTO change D34803 to track local linkage GlobalValues, and port the code to ThinLTO (GlobalAliases are not handled.)

Is the part about GlobalAliases not handled stale now with the most recent update?

llvm/lib/IR/Verifier.cpp

849 ↗

(On Diff #469457)

Would probably need to update https://llvm.org/docs/LangRef.html#aliases too.
Should we be checking that if alias is available_externally, then its aliasee is as well?

llvm/lib/Transforms/IPO/FunctionImport.cpp

1160

Would an alternative be converting the alias to a declaration? Slight pessimization in optimization ability but the verifier and other documentation doesn't need to be change.

But what if this was the prevailing copy of the alias? Could that happen?

1162

Should that be asserted?

[MaskRay]

llvm/lib/IR/Verifier.cpp

849 ↗

(On Diff #469457)

LLParser.cpp still rejects available_externally aliases. so available_externally alias is currently for LTO internal use, not exposed in .ll files.

llvm/lib/Transforms/IPO/FunctionImport.cpp

1160

Currently a GlobalAlias cannot alias a declaration or an available_externally. We need to make an extension and I feel that allowing available_externally is a better choice as it enables optimizations.

But what if this was the prevailing copy of the alias? Could that happen?

If a bitcode does something unsupported thing like making one comdat prevailing while another comdat non-prevailing and creating references between the two, such an unsupported case can happen. There is probably not much we can do and I don't think we need extra efforts to detect the case.

1162

The case technically can happen and will cause a Verifier failure, so we don't necessarily assert it here...

[tejohnson]

llvm/lib/Transforms/IPO/FunctionImport.cpp

1160

Currently a GlobalAlias cannot alias a declaration or an available_externally. We need to make an extension and I feel that allowing available_externally is a better choice as it enables optimizations.

I meant make the alias itself a declaration, not its aliasee. Since we are making it available_externally it must have a def elsewhere. Making it a declaration instead would avoid having to allow GlobalAlias to be available_externally in the verifier.

But what if this was the prevailing copy of the alias? Could that happen?

If a bitcode does something unsupported thing like making one comdat prevailing while another comdat non-prevailing and creating references between the two, such an unsupported case can happen. There is probably not much we can do and I don't think we need extra efforts to detect the case.

I think it would be the linker not the bitcode that would cause this situation to happen if at all? Not sure if we have a solid way to detect here what is prevailing (although if we have bumped the linkage from linkonce_odr to weak_odr then it was the prevailing copy). In the C5/D5 constructor alias case that failed, what was the linkage type before and after FinalizeInModule? The added test case has multiple weak_odr, but I would imagine that normally these would start being linkonce_odr?

1162

Ok as long as it fails obviously somewhere. Wondering if we need a TODO here to handle that case in the future?

[MaskRay]

llvm/lib/Transforms/IPO/FunctionImport.cpp

1160

I meant make the alias itself a declaration, not its aliasee. Since we are making it available_externally it must have a def elsewhere. Making it a declaration instead would avoid having to allow GlobalAlias to be available_externally in the verifier.

An alias cannot be a declaration.

If we use a function or variable, we need to figure out the aliasee of the alias to decide whether to use GlobalVariable or a Function. This is more work than using an available_externally alias.

If a C5/D5 constructor is non-prevailing, we expect that everything in the non-prevailing $_ZN1AD2Ev and the associated $_ZN1AD5Ev to be non-prevailing. The current code satisfies this property.

1162

Aliasing a ConstantExpr is a less-used extension, so I think a TODO is not necessary (the comment is still clear, just without the TODO keyword).

[tejohnson]

llvm/lib/Transforms/IPO/FunctionImport.cpp

1160

An alias cannot be a declaration.

If we use a function or variable, we need to figure out the aliasee of the alias to decide whether to use GlobalVariable or a Function. This is more work than using an available_externally alias.

Right, I meant drop to a variable or function declaration. We already have a helper to do so, defined in this file (see convertToDeclaration), that handles the alias case. Actually, the comments at the callsite (in this function) points out a case where we may need to do this for correctness (original linkage type is interposable) - not sure if that can ever happen here but would be good to handle correctly or assert.

If a C5/D5 constructor is non-prevailing

But we don't know for sure here whether it is prevailing or not, I think? In the C5/D5 case that was failing, what were the linkage types before and after FinalizeInModule above?

[MaskRay]

llvm/lib/Transforms/IPO/FunctionImport.cpp

1160

Unfortunately, convertToDeclaration is insufficient (this fact motivated me to use available_externally alias). It is for GlobalValues which will not call auto GS = DefinedGlobals.find(GV.getGUID()); in thinLTOInternalizeModule (as an empty name triggers a fault in getGUID). Non-prevailing GlobalAlias can exercise the fault code path.

If we really want to use a declaration, we need to make distinction of GlobalVariable/Function and remove the old GlobalAlias in TheModule.getAliasList() and add a new one to TheModule.getGlobalList() (or the Function counterpart). This is lots of complexity.

If a C5/D5 constructor is non-prevailing

But we don't know for sure here whether it is prevailing or not, I think? In the C5/D5 case that was failing, what were the linkage types before and after FinalizeInModule above?

We know for sure. C2/C5 must either be all prevailing or all non-prevailing. If we the available_externally code path is triggered for one alias in C2, then we know everything in C2/C5 should be available_externally as well. I verified this with some internal targets which motivated me to revert the previous patch.

[rnk]

llvm/lib/Transforms/IPO/FunctionImport.cpp

1160

If we really want to use a declaration, we need to make distinction of GlobalVariable/Function and remove the old GlobalAlias in TheModule.getAliasList() and add a new one to TheModule.getGlobalList() (or the Function counterpart). This is lots of complexity.

It doesn't sound so bad to me, but maybe I don't have the right visibility. As it stands, I'd rather have complex code than a less accurate IR model.

[tejohnson]

llvm/lib/Transforms/IPO/FunctionImport.cpp

1160

Unfortunately, convertToDeclaration is insufficient (this fact motivated me to use available_externally alias). It is for GlobalValues which will not call auto GS = DefinedGlobals.find(GV.getGUID()); in thinLTOInternalizeModule (as an empty name triggers a fault in getGUID). Non-prevailing GlobalAlias can exercise the fault code path.

Sorry, you lost me. What does DefinedGlobals have to do with calling convertToDeclaration, which takes a GV? The GlobalAlias below is a GV, why can't we just call convertToDeclaration on it instead of making it available_externally?

If we really want to use a declaration, we need to make distinction of GlobalVariable/Function and remove the old GlobalAlias in TheModule.getAliasList() and add a new one to TheModule.getGlobalList() (or the Function counterpart). This is lots of complexity.

convertToDeclaration handles converting alias to either a variable or function, and the underlying methods used to get one of those does add it to the appropriate list on the module. Not sure about removing from the alias list, but it does do the RAUW.

We know for sure. C2/C5 must either be all prevailing or all non-prevailing. If we the available_externally code path is triggered for one alias in C2, then we know everything in C2/C5 should be available_externally as well. I verified this with some internal targets which motivated me to revert the previous patch.

I agree they should all get the same prevailingness, if things are working as expected, but it sounds like they are in different comdat groups. I'm trying to understand what the linkage type was because if it started out as linkonce_odr the thin link should convert to either available_externally if not prevailing or weak_odr if prevailing.

[MaskRay]

llvm/lib/Transforms/IPO/FunctionImport.cpp

1160

An available_externally alias preserve the most information: it is still an alias and the aliasee unknown. Changing GlobalAlias to a GlobalVariable/Function declaration changes the shape drops the information. To me an available_externally GlobalAlias is a better choice here.

[MaskRay]

llvm/lib/Transforms/IPO/FunctionImport.cpp

1160

Sorry, you lost me. What does DefinedGlobals have to do with calling convertToDeclaration, which takes a GV? The GlobalAlias below is a GV, why can't we just call convertToDeclaration on it instead of making it available_externally?

convertToDeclaration handles converting alias to either a variable or function, and the underlying methods used to get one of those does add it to the appropriate list on the module. Not sure about removing from the alias list, but it does do the RAUW.

Sorry, it's not DefinedGlobals.

thinLTOInternalizeModule is a pass after thinLTOFinalizeInModule.
convertToDeclaration does RAUW, but does not remove the GlobalAlias from the module getAliasList.
An unnamed GlobalAlias (due to NewGV->takeName(&GV); in convertToDeclaration) causes a failure in thinLTOInternalizeModule=>internalizeModule=>Internalize.cpp:266=>Internalize.cpp:163 (if (shouldPreserveGV(GV))`).
The function tries to acceoss the first byte of the name while the name is empty.

I think this can be fixed by replacing a GlobalAlias with a GlobalAlias/Function declaration. However, the GlobalAlias/Function approach does not scale when we ever need to support a GlobalAlias which references a ConstantExpr (instead of a GlobalValue).
An available_externally GlobalAlias can be extended to such a use case. Though I cannot think of a case Clang does such codegen.

I agree they should all get the same prevailingness, if things are working as expected, but it sounds like they are in different comdat groups. I'm trying to understand what the linkage type was because if it started out as linkonce_odr the thin link should convert to either available_externally if not prevailing or weak_odr if prevailing.

The new test file constructor-alias.ll describes the case. It is derived from this C++ source file https://reviews.llvm.org/D135427#3869003

[tejohnson]

llvm/lib/Transforms/IPO/FunctionImport.cpp

1160

I see. Ok, I spent some time looking at the other callers of convertToDeclaration, since it seems as though they should have the same problem. The callsite in this file has a big FIXME (line 1107) that explains why we don't hit this - we currently explicitly avoid modifying GlobalAlias in thinLTOResolvePrevailingGUID, so that we don't end up with any that are even transiently available_externally, because the compiler doesn't handle that for aliases as we know also from your patch, so we never end up calling convertToDeclaration at that callsite for GlobalAlias.

The 2 other callsites are:

• dropDeadSymbols: also called from the ThinLTO backend just before this method, follows up with the deletion of affected symbols from the module (for aliases since convertToDeclaration does a RAUW the old GV should never have any uses left, so it should always get erased from module there).
• filterModule: called when writing the bitcode at the very end of the pre-LTO link compile. It erases the original GV from the module if convertToDeclaration returns false, which it does in the case of an alias where the symbol was replaced. So aliases are handled correctly there.

Given the FIXME in the earlier callsite, I think it would be good to go ahead and just add support for deleting the alias when it has been converted to a decl. This avoids having to loosen up the constraints in the verifier. In theory there could be some lost opportunities e.g. for inlining, but I would guess that in the comdat case we would have been able to capitalize on most available inlining opportunities in the pre-LTO compile's inlining step and not need something like function importing first anyway, so dropping to a decl here and removing the old alias seems safest.

Ideally given the issue you hit it would be safest to delete the original alias in convertToDeclaration after the RAUW. Then dropDeadSymbols just needs to be changed somewhat since it is adding the symbol to a set before calling convertToDeclaration - it can just guard adding the symbol to the set based on the return value of convertToDeclaration. And the callsite in filterModule should be changed to skip the erasure and just ignore the return value. This will be a nice cleanup anyway.

The new test file constructor-alias.ll describes the case. It is derived from this C++ source file https://reviews.llvm.org/D135427#3869003

Ok. I couldn't get it to generate the C5/D5 alias for me, even with cc1 -mconstructor-aliases, but in any case from the test case IR here it looks like they are getting generated as weak_odr. But also after looking at the FIXME from line 1107 I realized that because these are aliases we wouldn't have already modified their linkage type anyway.

[MaskRay]

llvm/lib/Transforms/IPO/FunctionImport.cpp

1160

Ideally given the issue you hit it would be safest to delete the original alias in convertToDeclaration after the RAUW. Then dropDeadSymbols just needs to be changed somewhat since it is adding the symbol to a set before calling convertToDeclaration - it can just guard adding the symbol to the set based on the return value of convertToDeclaration. And the callsite in filterModule should be changed to skip the erasure and just ignore the return value. This will be a nice cleanup anyway.

A GlobalAlias can alias a GlobalAlias, ultimately (via the alias chain) it aliases to a GlobalVariable or Function or GlobalIFunc or ConstantExpr. I see that convertToDeclaration can make GlobalAlias work for GlobalVariable/Function, and with some code GlobalIFunc can be supported, but how to support ConstantExpr?

available_externally GlobalAlias seems a natural extension and supports ConstantExpr well (if we find a need for it).

The new test file constructor-alias.ll describes the case. It is derived from this C++ source file https://reviews.llvm.org/D135427#3869003

Ok. I couldn't get it to generate the C5/D5 alias for me, even with cc1 -mconstructor-aliases, but in any case from the test case IR here it looks like they are getting generated as weak_odr. But also after looking at the FIXME from line 1107 I realized that because these are aliases we wouldn't have already modified their linkage type anyway.

Sorry, updated https://reviews.llvm.org/D135427#3869003 to include a correct C++ example using C5/D5.

[MaskRay]

Update LangRef.rst and LLParser for available_externally GlobalAlias

[nikic]

In D135427#3906831, @MaskRay wrote:

Update LangRef.rst and LLParser for available_externally GlobalAlias

This should go into a separate patch, which is not mixed up with LTO changes.

[MaskRay]

Depend on D137441

[tejohnson]

llvm/lib/Transforms/IPO/FunctionImport.cpp

1162

Thinking more about this, if we know that a constant expr aliasee will lead to a verifier failure, I think it is best to assert here so that it is easier to debug when and if that happens.

[MaskRay]

rebase
add an assert (reachable in a corner case, but ok to miss - Verifier.cpp will catch it anyway. assert just improves diagnostics).

[tejohnson]

lgtm

[dblaikie]

"can happen and will cause a Verifier failure" - could you clarify further what that means? A pass, given verified IR, shouldn't ever produce verifier-failing IR, right?

[MaskRay]

In D135427#3910121, @dblaikie wrote:

"can happen and will cause a Verifier failure" - could you clarify further what that means? A pass, given verified IR, shouldn't ever produce verifier-failing IR, right?

GlobalAlias in a non-prevailing COMDAT is not correctly handled today (may crash or spuriously succeed). This patch makes it correct for likely all cases Clang CodeGen produces (when C5/D5 constructor/destructor aliases are used).
But I do not plan to handle ConstantExpr aliasee (an extension espindola added in ~2015/2016 which allows .set test5, test2-bar) in a non-prevailing COMDAT - which to the best of my knowledge not used today.
That code still crashes, even not caught by the assert in FuncImport.cpp; I added a check in Verifier.cpp to catch to avoid a crash further down in the compilation pipeline.

[dblaikie]

In D135427#3910193, @MaskRay wrote:

In D135427#3910121, @dblaikie wrote:

"can happen and will cause a Verifier failure" - could you clarify further what that means? A pass, given verified IR, shouldn't ever produce verifier-failing IR, right?

GlobalAlias in a non-prevailing COMDAT is not correctly handled today (may crash or spuriously succeed). This patch makes it correct for likely all cases Clang CodeGen produces (when C5/D5 constructor/destructor aliases are used).

Ah, OK, sorry - incremental improvement then, fair enough. Thanks for the context!

[aeubanks]

llvm/lib/Transforms/IPO/FunctionImport.cpp

1166

we're hitting this assert in a bootstrap build of clang with ThinLTO on windows: https://crbug.com/1382839

[MaskRay]

llvm/lib/Transforms/IPO/FunctionImport.cpp

1166

How is the non-GlobalValue currently used? This feature in a COMDAT was not properly supported...

[ayzhao]

llvm/lib/Transforms/IPO/FunctionImport.cpp

1166

Not sure if this is helpful, but I loaded lldb-link.exe into a debugger, and calling GA.dump() when I hit this assertion resulted in the following:

@"??_7bad_array_new_length@stdext@@6B@" = unnamed_addr alias ptr, getelementptr inbounds ({ [4 x ptr] }, ptr @anon.2dbfeeb171d3ed35dcf0e9128a5a5941.7, i32 0, i32 0, i32 1)

FYI repro instructions are here: https://crbug.com/1382839#c6

[ayzhao]

llvm/lib/Transforms/IPO/FunctionImport.cpp

1166

This seems to be blowing up in the gmock module:

Debug.Print TheModule.getModuleIdentifier().c_str()
0x00000227ea758630 "lib/llvm_gtest.libgmock-all.cc.obj1929440"

[aeubanks]

IR from building gmock-all.cc on windows with ThinLTO

a.ll2 MBDownload

[MaskRay]

(https://bugs.chromium.org/p/chromium/issues/detail?id=1382839#c6 is probably useful for chromium developers, but I do not know how to build chrome with the instruction...)

[MaskRay]

Switch to GA.getAliaseeObject() to work around a Windows vftable hack (see updated summary): ThinLTO does not handle the alias correctly, the comdat is discarded while the global isn't marked as available_externally. It appears to be benign and the latest diff uses a way to retain the behavior.

[aeubanks]

In D135427#3920695, @MaskRay wrote:

(https://bugs.chromium.org/p/chromium/issues/detail?id=1382839#c6 is probably useful for chromium developers, but I do not know how to build chrome with the instruction...)

it wasn't clear, but that was just a typical bootstrap build of clang on windows with ThinLTO, nothing to do with chrome itself

anyway, verified that this latest patch fixes the issue

llvm/lib/LTO/LTO.cpp

 handleNonPrevailingComdat(GlobalValue &GV,
                           std::set<const Comdat *> &NonPrevailingComdats) {
   Comdat *C = GV.getComdat();
   if (!C)
     return;
 
   if (!NonPrevailingComdats.count(C))
     return;
 
-  // Additionally need to drop externally visible global values from the comdat
-  // to available_externally, so that there aren't multiply defined linker
-  // errors.
-  if (!GV.hasLocalLinkage())
-    GV.setLinkage(GlobalValue::AvailableExternallyLinkage);
+  // Additionally need to drop all global values from the comdat to
+  // available_externally, to satisfy the COMDAT requirement that all members
+  // are discarded as a unit. The non-local linkage global values avoid
+  // duplicate definition linker errors.
 
   if (auto GO = dyn_cast<GlobalObject>(&GV))
     GO->setComdat(nullptr);
 }
 
 // Add a regular LTO object to the link.
 // The resulting module needs to be linked into the combined LTO module with
 // linkRegularLTO.

llvm/lib/Transforms/IPO/FunctionImport.cpp

bool llvm::convertToDeclaration(GlobalValue &GV) {
   if (!GV.isImplicitDSOLocal())
     GV.setDSOLocal(false);
   return true;
 }
 
 void llvm::thinLTOFinalizeInModule(Module &TheModule,
                                    const GVSummaryMapTy &DefinedGlobals,
                                    bool PropagateAttrs) {
+  DenseSet<Comdat *> NonPrevailingComdats;
   auto FinalizeInModule = [&](GlobalValue &GV, bool Propagate = false) {
     // See if the global summary analysis computed a new resolved linkage.
     const auto &GS = DefinedGlobals.find(GV.getGUID());
     if (GS == DefinedGlobals.end())
       return;
 
     if (Propagate)
       if (FunctionSummary *FS = dyn_cast<FunctionSummary>(GS->second)) {
if (GlobalValue::isAvailableExternallyLinkage(NewLinkage) &&
                         << "` from " << GV.getLinkage() << " to " << NewLinkage
                         << "\n");
       GV.setLinkage(NewLinkage);
     }
     // Remove declarations from comdats, including available_externally
     // as this is a declaration for the linker, and will be dropped eventually.
     // It is illegal for comdats to contain declarations.
     auto *GO = dyn_cast_or_null<GlobalObject>(&GV);
-    if (GO && GO->isDeclarationForLinker() && GO->hasComdat())
+    if (GO && GO->isDeclarationForLinker() && GO->hasComdat()) {
+      NonPrevailingComdats.insert(GO->getComdat());
       GO->setComdat(nullptr);
+    }
   };
 
   // Process functions and global now
   for (auto &GV : TheModule)
     FinalizeInModule(GV, PropagateAttrs);
   for (auto &GV : TheModule.globals())
     FinalizeInModule(GV);
   for (auto &GV : TheModule.aliases())
     FinalizeInModule(GV);
+
+  // For a non-prevailing comdat, all its members must be available_externally.
+  // FinalizeInModule has handled non-local-linkage GlobalValues. Here we handle
+  // local linkage GlobalValues.
+  if (NonPrevailingComdats.empty())
+    return;
+  for (auto &GO : TheModule.global_objects()) {
+    if (auto *C = GO.getComdat(); C && NonPrevailingComdats.count(C)) {
+      GO.setComdat(nullptr);
+      GO.setLinkage(GlobalValue::AvailableExternallyLinkage);
+    }
+  }
 }
 
 /// Run internalization on \p TheModule based on symmary analysis.
 void llvm::thinLTOInternalizeModule(Module &TheModule,
                                     const GVSummaryMapTy &DefinedGlobals) {
   // Declare a callback for the internalize pass that will ask for every
   // candidate GlobalValue if it can be internalized or not.
   auto MustPreserveGV = [&](const GlobalValue &GV) -> bool {
     // It may be the case that GV is on a chain of an ifunc, its alias and
     // subsequent aliases. In this case, the summary for the value is not
     // available.
     if (isa<GlobalIFunc>(&GV) ||
         (isa<GlobalAlias>(&GV) &&
          isa<GlobalIFunc>(cast<GlobalAlias>(&GV)->getAliaseeObject())))
       return true;
 
     // Lookup the linkage recorded in the summaries during global analysis.
     auto GS = DefinedGlobals.find(GV.getGUID());

llvm/test/LTO/Resolution/X86/comdat-mixed-lto.ll

 ; Test of comdat handling with mixed thinlto and regular lto compilation.
 
 ; This module is compiled with ThinLTO
 ; RUN: opt -module-summary -o %t1.o %s
 ; Input module compiled for regular LTO
 ; RUN: opt -o %t2.o %p/Inputs/comdat-mixed-lto.ll
 
 ; The copy of C from this module is prevailing. The copy of C from the
 ; regular LTO module is not prevailing, and will be dropped to
 ; available_externally.
-; RUN: llvm-lto2 run -r=%t1.o,C,pl -r=%t2.o,C,l -r=%t2.o,testglobfunc,lxp -r=%t1.o,testglobfunc,lx -o %t3 %t1.o %t2.o -save-temps
+; RUN: llvm-lto2 run -r=%t1.o,C,pl -r=%t2.o,C,l -r=%t1.o,testglobfunc,lxp -r=%t2.o,testglobfunc,lx -o %t3 %t1.o %t2.o -save-temps
 
 ; The Input module (regular LTO) is %t3.0. Check to make sure that we removed
 ; __cxx_global_var_init and testglobfunc from comdat. Also check to ensure
 ; that testglobfunc was dropped to available_externally. Otherwise we would
 ; have linker multiply defined errors as it is no longer in a comdat and
 ; would clash with the copy from this module.
 ; RUN: llvm-dis %t3.0.0.preopt.bc -o - | FileCheck %s
-; CHECK: define internal void @__cxx_global_var_init() section ".text.startup" {
-; CHECK: define available_externally dso_local void @testglobfunc() section ".text.startup" {
+
+; CHECK: @llvm.global_ctors = appending global [1 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 65535, ptr @__cxx_global_var_init, ptr @C }]
+; CHECK: @C = available_externally dso_local global %"class.Test::ptr" zeroinitializer, align 4
+; CHECK-NOT: declare
+; CHECK: declare dso_local void @__cxx_global_var_init() section ".text.startup"
+; CHECK-NOT: declare
+
+; Check the behavior with the prevailing testglobfunc in %t2.o.
+; RUN: llvm-lto2 run -r=%t1.o,C,pl -r=%t2.o,C,l -r=%t1.o,testglobfunc,lx -r=%t2.o,testglobfunc,plx -o %t4 %t1.o %t2.o -save-temps
+; RUN: llvm-dis %t4.0.0.preopt.bc -o - | FileCheck %s --check-prefix=CHECK2
+
+; CHECK2: @llvm.global_ctors = appending global [1 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 65535, ptr @__cxx_global_var_init, ptr @C }]
+; CHECK2: @C = available_externally dso_local global %"class.Test::ptr" zeroinitializer, align 4
+; CHECK2: declare dso_local void @__cxx_global_var_init() section ".text.startup"
+; CHECK2: define available_externally dso_local void @testglobfunc() section ".text.startup" {
 
 ; ModuleID = 'comdat-mixed-lto.o'
 source_filename = "comdat-mixed-lto.cpp"
 target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 %"class.Test::ptr" = type { i32 }
 

llvm/test/ThinLTO/X86/Inputs/linkonce_resolution_comdat.ll

 target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
-$c2 = comdat any
+$f = comdat any
+$g = comdat any
 
-define linkonce_odr i32 @f(i8*) unnamed_addr comdat($c2) {
+@g_private = private global i32 41, comdat($g)
+
+define linkonce_odr i32 @f(i8*) unnamed_addr comdat($f) {
+    ret i32 41
+}
+
+define linkonce_odr i32 @g() unnamed_addr comdat($g) {
     ret i32 41
 }
 
-define i32 @g() {
+define internal void @g_internal() unnamed_addr comdat($g) {
+    ret void
+}
+
+define i32 @h() {
     %i = call i32 @f(i8* null)
     ret i32 %i
 }

llvm/test/ThinLTO/X86/linkonce_resolution_comdat.ll

-; This test ensures that we drop the preempted copy of @f from %t2.bc from its
-; comdat after making it available_externally. If not we would get a
-; verification error.
+; This test ensures that we drop the preempted copy of @f/@g from %t2.bc from their
+; comdats after making it available_externally. If not we would get a
+; verification error. g_internal/g_private are changed to available_externally
+; as well since it is in the same comdat of g.
 ; RUN: opt -module-summary %s -o %t1.bc
 ; RUN: opt -module-summary %p/Inputs/linkonce_resolution_comdat.ll -o %t2.bc
-; RUN: llvm-lto -thinlto-action=run -disable-thinlto-funcattrs=0 %t1.bc %t2.bc -exported-symbol=f -exported-symbol=g -thinlto-save-temps=%t3.
+; RUN: llvm-lto -thinlto-action=run -disable-thinlto-funcattrs=0 %t1.bc %t2.bc -exported-symbol=f -exported-symbol=g -exported-symbol=h -thinlto-save-temps=%t3.
 
 ; RUN: llvm-dis %t3.0.3.imported.bc -o - | FileCheck %s --check-prefix=IMPORT1
 ; RUN: llvm-dis %t3.1.3.imported.bc -o - | FileCheck %s --check-prefix=IMPORT2
 ; Copy from first module is prevailing and converted to weak_odr, copy
 ; from second module is preempted and converted to available_externally and
 ; removed from comdat.
-; IMPORT1: define weak_odr i32 @f(i8* %0) unnamed_addr [[ATTR:#[0-9]+]] comdat($c1) {
+; IMPORT1: @g_private = private global i32 43, comdat($g)
+; IMPORT1: define weak_odr i32 @f(i8* %0) unnamed_addr [[ATTR:#[0-9]+]] comdat {
+; IMPORT1: define weak_odr i32 @g() unnamed_addr [[ATTR]] comdat {
+; IMPORT1: define internal void @g_internal() unnamed_addr comdat($g) {
+
+; IMPORT2: @g_private = available_externally dso_local global i32 41{{$}}
 ; IMPORT2: define available_externally i32 @f(i8* %0) unnamed_addr [[ATTR:#[0-9]+]] {
+; IMPORT2: define available_externally i32 @g() unnamed_addr [[ATTR]] {
+; IMPORT2: define available_externally dso_local void @g_internal() unnamed_addr {
 
 ; CHECK-DAG: attributes [[ATTR]] = { norecurse nounwind }
 
-; RUN: llvm-nm -o - < %t1.bc.thinlto.o | FileCheck %s --check-prefix=NM1
+; RUN: llvm-nm %t1.bc.thinlto.o | FileCheck %s --check-prefix=NM1
 ; NM1: W f
+; NM1: W g
 
-; RUN: llvm-nm -o - < %t2.bc.thinlto.o | FileCheck %s --check-prefix=NM2
+; RUN: llvm-nm %t2.bc.thinlto.o | FileCheck %s --check-prefix=NM2
 ; f() would have been turned into available_externally since it is preempted,
-; and inlined into g()
+; and inlined into h()
 ; NM2-NOT: f
+; NM2-NOT: g
 
 target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
-$c1 = comdat any
+$f = comdat any
+$g = comdat any
+
+@g_private = private global i32 43, comdat($g)
 
-define linkonce_odr i32 @f(i8*) unnamed_addr comdat($c1) {
+define linkonce_odr i32 @f(i8*) unnamed_addr comdat {
     ret i32 43
 }
+
+define linkonce_odr i32 @g() unnamed_addr comdat {
+    ret i32 43
+}
+
+define internal void @g_internal() unnamed_addr comdat($g) {
+    ret void
+}