[clang] [Clang][CodeGen] Fix use of CXXThisValue with StrictVTablePointers (PR #68169)
https://github.com/mizvekov created https://github.com/llvm/llvm-project/pull/68169 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 https://github.com/llvm/llvm-project/issues/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. >From 456b3d4e38fb80c599959d6b1ff5c93d94232394 Mon Sep 17 00:00:00 2001 From: Matheus Izvekov Date: Wed, 4 Oct 2023 02:34:31 +0200 Subject: [PATCH 1/2] [Clang][CodeGen][NFC] Add (broken) test case for GH67937 This adds a test case for yet unfixed https://github.com/llvm/llvm-project/issues/67937 --- .../strict-vtable-pointers-GH67937.cpp| 29 +++ 1 file changed, 29 insertions(+) create mode 100644 clang/test/CodeGenCXX/strict-vtable-pointers-GH67937.cpp 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 000..6fb1e68c288b181 --- /dev/null +++ b/clang/test/CodeGenCXX/strict-vtable-pointers-GH67937.cpp @@ -0,0 +1,29 @@ +// RUN: %clang_cc1 %s -I%S -triple=x86_64-pc-windows-msvc -fstrict-vtable-pointers -disable-llvm-passes -disable-llvm-verifier -O1 -emit-llvm -o %t.ll +// RUN: FileCheck %s < %t.ll +// RUN: not llvm-as < %t.ll -o /dev/null 2>&1 | FileCheck %s --check-prefix=CHECK-VERIFIER + +struct A { + virtual ~A(); +}; +struct B : virtual A {}; +class C : B {}; +C foo; + +// FIXME: This is not supposed to generate invalid IR! +// CHECK-VERIFIER: Instruction does not dominate all uses! +// CHECK-VERIFIER-NEXT: %1 = call ptr @llvm.launder.invariant.group.p0(ptr %this1) +// CHECK-VERIFIER-NEXT: %3 = call ptr @llvm.launder.invariant.group.p0(ptr %1) + +// 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: +// FIXME: Should be using '%this1' instead of %1 below. +// CHECK-NEXT: %3 = call ptr @llvm.launder.invariant.group.p0(ptr %1) +// CHECK-NEXT: %call3 = call noundef ptr @"??0B@@QEAA@XZ"(ptr {{.*}} %3, i32 noundef 0) #2 >From 4073aca154fe55e56b0025509ce85690eba061df Mon Sep 17 00:00:00 2001 From: Matheus Izvekov Date: Wed, 4 Oct 2023 02:40:25 +0200 Subject: [PATCH 2/2] [Clang][CodeGen] Fix use of CXXThisValue with StrictVTablePointers 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 https://github.com/llvm/llvm-project/issues/67937 --- clang/lib/CodeGen/CGClass.cpp | 7 +++ .../CodeGenCXX/strict-vtable-pointers-GH67937.cpp | 11 ++- 2 files changed, 5 insertions(+), 13 deletions(-) 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 @@ -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 = Ol
[clang] [Clang][CodeGen] Fix use of CXXThisValue with StrictVTablePointers (PR #68169)
llvmbot wrote: @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 https://github.com/llvm/llvm-project/issues/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 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 @@ -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 000..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 `` https://github.com/llvm/llvm-project/pull/68169 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][CodeGen] Fix use of CXXThisValue with StrictVTablePointers (PR #68169)
https://github.com/rnk approved this pull request. 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. https://github.com/llvm/llvm-project/pull/68169 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][CodeGen] Fix use of CXXThisValue with StrictVTablePointers (PR #68169)
https://github.com/mizvekov updated https://github.com/llvm/llvm-project/pull/68169 >From bd6cb19d6261b46ff1c7014140eff59609134272 Mon Sep 17 00:00:00 2001 From: Matheus Izvekov Date: Wed, 4 Oct 2023 02:40:25 +0200 Subject: [PATCH] [Clang][CodeGen] Fix use of CXXThisValue with StrictVTablePointers 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 https://github.com/llvm/llvm-project/issues/67937 --- clang/lib/CodeGen/CGClass.cpp | 7 +++ .../CodeGenCXX/strict-vtable-pointers-GH67937.cpp | 11 ++- 2 files changed, 5 insertions(+), 13 deletions(-) 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 @@ -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 index 6fb1e68c288b181..692ca23a69abe6a 100644 --- a/clang/test/CodeGenCXX/strict-vtable-pointers-GH67937.cpp +++ b/clang/test/CodeGenCXX/strict-vtable-pointers-GH67937.cpp @@ -1,6 +1,5 @@ -// RUN: %clang_cc1 %s -I%S -triple=x86_64-pc-windows-msvc -fstrict-vtable-pointers -disable-llvm-passes -disable-llvm-verifier -O1 -emit-llvm -o %t.ll +// 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 -// RUN: not llvm-as < %t.ll -o /dev/null 2>&1 | FileCheck %s --check-prefix=CHECK-VERIFIER struct A { virtual ~A(); @@ -9,11 +8,6 @@ struct B : virtual A {}; class C : B {}; C foo; -// FIXME: This is not supposed to generate invalid IR! -// CHECK-VERIFIER: Instruction does not dominate all uses! -// CHECK-VERIFIER-NEXT: %1 = call ptr @llvm.launder.invariant.group.p0(ptr %this1) -// CHECK-VERIFIER-NEXT: %3 = call ptr @llvm.launder.invariant.group.p0(ptr %1) - // 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 @@ -24,6 +18,5 @@ C foo; // CHECK-NEXT: br label %ctor.skip_vbases // CHECK-EMPTY: // CHECK-NEXT: ctor.skip_vbases: -// FIXME: Should be using '%this1' instead of %1 below. -// CHECK-NEXT: %3 = call ptr @llvm.launder.invariant.group.p0(ptr %1) +// 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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][CodeGen] Fix use of CXXThisValue with StrictVTablePointers (PR #68169)
https://github.com/mizvekov edited https://github.com/llvm/llvm-project/pull/68169 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][CodeGen] Fix use of CXXThisValue with StrictVTablePointers (PR #68169)
https://github.com/mizvekov closed https://github.com/llvm/llvm-project/pull/68169 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits