[MS-ABI] Remove comdat attribute for inheriting ctor.

[jyu2]

Summary

Currently, for MS, the linkage for the inheriting constructors is set to
internal. However, the comdat attribute is also set like:

define internal noundef ptr @"??0?$B@_N@@qeaa@AEBVF@@aebua@@@z"(ptr noundef nonnull returned align 1 dereferenceable(1) %this, ptr noundef nonnull align 1 dereferenceable(1) %0, ptr noundef nonnull align 1 dereferenceable(1) %1) unnamed_addr comdat

This could cause linker to fail.

The change is to remove comdat attribute for the inheriting constructor
to make linker happy.

[jyu2]

Ping,

Our customer fond problem I submit bug: in https://github.com/llvm/llvm-project/issues/65045

This issue related with big change of 5179eb78210a2ad01a18c37b75048ccfe78414ac

Where internal linkage for inheriting ctor were set in change set at following code. Could some one help to code review the change? Or any suggestion on how to fix this in other ways?

Thank you so much for the help.
Jennifer

CodeGenModule.cpp
if (isa<CXXConstructorDecl>(D) &&

    cast<CXXConstructorDecl>(D)->isInheritingConstructor() &&
    Context.getTargetInfo().getCXXABI().isMicrosoft()) {
  // Our approach to inheriting constructors is fundamentally different from
  // that used by the MS ABI, so keep our inheriting constructor thunks
  // internal rather than trying to pick an unambiguous mangling for them.
  return llvm::GlobalValue::InternalLinkage;
}

[rnk]

It took me a while to find this code to understand the issue. The problem is that there is an inconsistency between the GVA linkage and the LLVM IR linkage for these constructors. In this godbolt example, ??0?$TInputDataVertexModel.. is marked internal and marked comdat, and that is uncommon. PGO instrumentation appears to do the wrong thing in that case. Is that more or less what's happening?

The suggested fix here is to move this logic up from CodeGen into the earlier, "GVA level" linkage calculation, so we don't mark this thing comdat, right? The logic was added in this 2016 change from @rsmith :
https://github.com/llvm/llvm-project/commit/5179eb78210a2#diff-e724febedab9c1a2832bf2056d208ff02ddcb2e6f90b5a653afc9b19ac78a5d7R768

Ideally, I'd like to come up with our own Clang mangling and emit a thunk with linkonce_odr linkage or GVA_DisardableODR linkage, which means we need to understand why Richard thought this was necessary earlier.

Before we do that, you should be able to delete the logic in CodeGenModule::getFunctionLinkage to handle inheriting constructor thunks. Your change should have the same effect. Can you do that, and check that it passes tests? If we do that, we're don't have to accumulate extra special case code, we've just moved the special case code earlier, up from codegen into the earlier linkage calculation.

clang/test/CodeGenCXX/ms-inheriting-ctor.cpp

42

To make this less fragile, can you come up with a way to use CHECK-NOT: comdat since that's the key thing we're testing for here? You will need some subsequent anchor like entry: or something else.

[jyu2]

Thanks @rnk. This is address his comment.

[jyu2]

Hi @rnk,

Thank you so much for the suggestion.

Before we do that, you should be able to delete the logic in CodeGenModule::getFunctionLinkage to handle inheriting constructor thunks. Your change should have the same effect. Can you do that, and check that it passes tests? If we do that, we're don't have to accumulate extra special case code, we've just moved the special case code earlier, up from codegen into the earlier linkage calculation.

Yes, you are right. I remove the code in CodeGenModule.cpp, the test passes for check-clang.

clang/test/CodeGenCXX/ms-inheriting-ctor.cpp

42

Thanks. I changed.

[rnk]

lgtm with some tweaks to the test.

clang/test/CodeGenCXX/ms-inheriting-ctor.cpp

42

This isn't quite what I was trying to suggest, I was thinking something more like:

// CHECK-LABEL: define internal noundef ptr @"??0?$B@_N@@QEAA@AEBVF@@AEBUA@@@Z"
// CHECK-NOT: comdat
// CHECK-SAME: {{\{$}}

So, it finds the class with the inheriting constructor, and then checks that the word "comdat" does not appear anywhere on that line. I haven't tested the CHECK-SAME pattern, it may need some adjustment.

[jyu2]

Thanks. @rnk

clang/test/CodeGenCXX/ms-inheriting-ctor.cpp

42

I see. Thanks!! I changed.

clang/lib/AST/ASTContext.cpp

static GVALinkage basicGVALinkageForFunction(const ASTContext &Context,
   }
 
   // Functions specified with extern and inline in -fms-compatibility mode
   // forcibly get emitted.  While the body of the function cannot be later
   // replaced, the function definition cannot be discarded.
   if (FD->isMSExternInline())
     return GVA_StrongODR;
 
+  if (Context.getTargetInfo().getCXXABI().isMicrosoft() &&
+      isa<CXXConstructorDecl>(FD) &&
+      cast<CXXConstructorDecl>(FD)->isInheritingConstructor())
+    // Our approach to inheriting constructors is fundamentally different from
+    // that used by the MS ABI, so keep our inheriting constructor thunks
+    // internal rather than trying to pick an unambiguous mangling for them.
+    return GVA_Internal;
+
   return GVA_DiscardableODR;
 }
 
 static GVALinkage adjustGVALinkageForAttributes(const ASTContext &Context,
                                                 const Decl *D, GVALinkage L) {
   // See http://msdn.microsoft.com/en-us/library/xa0d9ste.aspx
   // dllexport/dllimport on inline functions.
   if (D->hasAttr<DLLImportAttr>()) {

clang/lib/CodeGen/CodeGenModule.cpp

 CodeGenModule::getFunctionLinkage(GlobalDecl GD) {
   const auto *D = cast<FunctionDecl>(GD.getDecl());
 
   GVALinkage Linkage = getContext().GetGVALinkageForFunction(D);
 
   if (const auto *Dtor = dyn_cast<CXXDestructorDecl>(D))
     return getCXXABI().getCXXDestructorLinkage(Linkage, Dtor, GD.getDtorType());
 
-  if (isa<CXXConstructorDecl>(D) &&
-      cast<CXXConstructorDecl>(D)->isInheritingConstructor() &&
-      Context.getTargetInfo().getCXXABI().isMicrosoft()) {
-    // Our approach to inheriting constructors is fundamentally different from
-    // that used by the MS ABI, so keep our inheriting constructor thunks
-    // internal rather than trying to pick an unambiguous mangling for them.
-    return llvm::GlobalValue::InternalLinkage;
-  }
-
   return getLLVMLinkageForDeclarator(D, Linkage);
 }
 
 llvm::ConstantInt *CodeGenModule::CreateCrossDsoCfiTypeId(llvm::Metadata *MD) {
   llvm::MDString *MDS = dyn_cast<llvm::MDString>(MD);
   if (!MDS) return nullptr;
 
   return llvm::ConstantInt::get(Int64Ty, llvm::MD5Hash(MDS->getString()));

clang/test/CodeGenCXX/ms-inheriting-ctor.cpp

+// RUN: %clang_cc1 -fcxx-exceptions -triple=x86_64-windows-msvc -emit-llvm %s -o - | FileCheck %s
+
+class F {
+public:
+  F(wchar_t *);
+};
+using a = F;
+struct A {};
+struct b {
+  b(a, F, A);
+};
+template <typename, typename> struct c : b {
+  c(const a &p1, const A &d) : b(p1, 0, d) {}
+};
+template <typename e> struct B : c<e, b> {
+  using c<e, b>::c;
+};
+class f {
+public:
+  f(...);
+}
+
+typedef g;
+class C {
+public:
+  C(g, f);
+};
+static wchar_t h;
+class D {
+public:
+  static C E();
+};
+
+C D::E() {
+  C i(B<bool>(&h, {}), f());
+  return i;
+}
+
+// Inheriting ctor has internal linkage without comdat.
+
+// CHECK-LABEL: define internal noundef ptr @"??0?$B@_N@@QEAA@AEBVF@@AEBUA@@@Z"
+// CHECK-NOT:comdat
+// CHECK-SAME: {{\{$}}
+
+// non-inheriting ctro should has linkonce_odr with comdat attribute.
+
+// CHECK-LABEL: define linkonce_odr dso_local noundef ptr @"??0?$c@_NUb@@@@QEAA@AEBVF@@AEBUA@@@Z"
+// CHECK:comdat
+// CHECK-SAME: {{\{$}}