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

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

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 <https://gcc.godbolt.org/z/nj8aarvj7>, `??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.



================
Comment at: clang/test/CodeGenCXX/ms-inheriting-ctor.cpp:41
+
+// CHECK-LABEL: define internal noundef ptr @"??0?$B at _N@@QEAA at 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 #2 align 2
+// CHECK-LABEL: define linkonce_odr dso_local noundef ptr @"??0?$c at _NUb@@@@QEAA at AEBVF@@AEBUA@@@Z"(ptr noundef nonnull returned align 1 dereferenceable(1) %this, ptr noundef nonnull align 1 dereferenceable(1) %p1, ptr noundef nonnull align 1 dereferenceable(1) %d) unnamed_addr #2 comdat align 2
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158538/new/

https://reviews.llvm.org/D158538