Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Clang][CodeGen] Fix use of CXXThisValue with StrictVTablePointers #68169

Merged
merged 1 commit into from
Oct 4, 2023

Conversation

mizvekov
Copy link
Contributor

@mizvekov mizvekov commented Oct 4, 2023

When emitting non-virtual base initializers for the constructor prologue,
we would potentially use a re-laundered this pointer value from a
previous block, which subsequently would not dominate this use.

With this fix, we always launder the original CXXThisValue.

This fixes #67937

@mizvekov mizvekov self-assigned this Oct 4, 2023
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:codegen labels Oct 4, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Oct 4, 2023

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Changes

When emitting non-virtual base initializers for the constructor prologue,
we would potentially use a re-laundered this pointer value from a
previous block, which subsequently would not dominate this use.

With this fix, we always launder the original CXXThisValue.

This fixes #67937

Note: First commit just adds the test, which will be fixed by the second commit. I will land the test separately after the first round.


Full diff: https://github.com/llvm/llvm-project/pull/68169.diff

2 Files Affected:

  • (modified) clang/lib/CodeGen/CGClass.cpp (+3-4)
  • (added) clang/test/CodeGenCXX/strict-vtable-pointers-GH67937.cpp (+22)
diff --git a/clang/lib/CodeGen/CGClass.cpp b/clang/lib/CodeGen/CGClass.cpp
index 57a424c6f176c47..d18f186ce5b4157 100644
--- a/clang/lib/CodeGen/CGClass.cpp
+++ b/clang/lib/CodeGen/CGClass.cpp
@@ -28,6 +28,7 @@
 #include "clang/CodeGen/CGFunctionInfo.h"
 #include "llvm/IR/Intrinsics.h"
 #include "llvm/IR/Metadata.h"
+#include "llvm/Support/SaveAndRestore.h"
 #include "llvm/Transforms/Utils/SanitizerStats.h"
 #include <optional>
 
@@ -1291,10 +1292,10 @@ void CodeGenFunction::EmitCtorPrologue(const CXXConstructorDecl *CD,
     assert(BaseCtorContinueBB);
   }
 
-  llvm::Value *const OldThis = CXXThisValue;
   for (; B != E && (*B)->isBaseInitializer() && (*B)->isBaseVirtual(); B++) {
     if (!ConstructVBases)
       continue;
+    SaveAndRestore ThisRAII(CXXThisValue);
     if (CGM.getCodeGenOpts().StrictVTablePointers &&
         CGM.getCodeGenOpts().OptimizationLevel > 0 &&
         isInitializerOfDynamicClass(*B))
@@ -1311,7 +1312,7 @@ void CodeGenFunction::EmitCtorPrologue(const CXXConstructorDecl *CD,
   // Then, non-virtual base initializers.
   for (; B != E && (*B)->isBaseInitializer(); B++) {
     assert(!(*B)->isBaseVirtual());
-
+    SaveAndRestore ThisRAII(CXXThisValue);
     if (CGM.getCodeGenOpts().StrictVTablePointers &&
         CGM.getCodeGenOpts().OptimizationLevel > 0 &&
         isInitializerOfDynamicClass(*B))
@@ -1319,8 +1320,6 @@ void CodeGenFunction::EmitCtorPrologue(const CXXConstructorDecl *CD,
     EmitBaseInitializer(*this, ClassDecl, *B);
   }
 
-  CXXThisValue = OldThis;
-
   InitializeVTablePointers(ClassDecl);
 
   // And finally, initialize class members.
diff --git a/clang/test/CodeGenCXX/strict-vtable-pointers-GH67937.cpp b/clang/test/CodeGenCXX/strict-vtable-pointers-GH67937.cpp
new file mode 100644
index 000000000000000..692ca23a69abe6a
--- /dev/null
+++ b/clang/test/CodeGenCXX/strict-vtable-pointers-GH67937.cpp
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 %s -I%S -triple=x86_64-pc-windows-msvc -fstrict-vtable-pointers -disable-llvm-passes -O1 -emit-llvm -o %t.ll
+// RUN: FileCheck %s < %t.ll
+
+struct A {
+  virtual ~A();
+};
+struct B : virtual A {};
+class C : B {};
+C foo;
+
+// CHECK-LABEL: define {{.*}} @"??0C@@QEAA@XZ"(ptr {{.*}} %this, i32 {{.*}} %is_most_derived)
+// CHECK: ctor.init_vbases:
+// CHECK-NEXT: %0 = getelementptr inbounds i8, ptr %this1, i64 0
+// CHECK-NEXT: store ptr @"??_8C@@7B@", ptr %0
+// CHECK-NEXT: %1 = call ptr @llvm.launder.invariant.group.p0(ptr %this1)
+// CHECK-NEXT: %2 = getelementptr inbounds i8, ptr %1, i64 8
+// CHECK-NEXT: %call = call noundef ptr @"??0A@@QEAA@XZ"(ptr {{.*}} %2) #2
+// CHECK-NEXT: br label %ctor.skip_vbases
+// CHECK-EMPTY:
+// CHECK-NEXT: ctor.skip_vbases:
+// CHECK-NEXT: %3 = call ptr @llvm.launder.invariant.group.p0(ptr %this1)
+// CHECK-NEXT: %call3 = call noundef ptr @"??0B@@QEAA@XZ"(ptr {{.*}} %3, i32 noundef 0) #2

@rnk rnk requested a review from aeubanks October 4, 2023 20:26
Copy link
Collaborator

@rnk rnk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to delegate the review to @aeubanks, who was the last one I'm aware of to touch the strict vptr feature, but I think this is a pretty straightforward fix.

When emitting non-virtual base initializers for the constructor prologue,
we would potentially use a re-laundered this pointer value from a
previous block, which subsequently would not dominate this use.

With this fix, we always launder the original CXXThisValue.

This fixes llvm#67937
@mizvekov mizvekov merged commit 5099dc3 into llvm:main Oct 4, 2023
3 checks passed
@mizvekov mizvekov deleted the fix_GH67937 branch October 4, 2023 21:45
Guzhu-AMD pushed a commit to GPUOpen-Drivers/llvm-project that referenced this pull request Oct 12, 2023
Local branch amd-gfx ab27814 Merged main:7539bcf994ed into amd-gfx:0e8798f5c1bd
Remote branch main 5099dc3 [Clang][CodeGen] Fix use of CXXThisValue with StrictVTablePointers (llvm#68169)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:codegen clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Clang/CodeGen] Ill-formed LLVM generated with -fstrict-vtable-pointers for MSVC target
3 participants