llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clangir Author: Andy Kaylor (andykaylor) <details> <summary>Changes</summary> When we call `getLoc()` with an invalid `SourceLocation` and `currSrcLoc` is also invalid, we were crashing or asserting. I tracked down one case where this was happening (generating an argument in a vtable thunk) and fixed that to provide a location. I also am updating the `getLoc()` implementation so that it will use an unknown location in release builds rather than crashing because the location isn't critical for correct compilation. --- Full diff: https://github.com/llvm/llvm-project/pull/185059.diff 3 Files Affected: - (modified) clang/lib/CIR/CodeGen/CIRGenFunction.cpp (+12-4) - (modified) clang/lib/CIR/CodeGen/CIRGenVTables.cpp (+2-2) - (modified) clang/test/CIR/CodeGen/thunks.cpp (+64) ``````````diff diff --git a/clang/lib/CIR/CodeGen/CIRGenFunction.cpp b/clang/lib/CIR/CodeGen/CIRGenFunction.cpp index c64630db08965..71a647e37ea52 100644 --- a/clang/lib/CIR/CodeGen/CIRGenFunction.cpp +++ b/clang/lib/CIR/CodeGen/CIRGenFunction.cpp @@ -113,9 +113,14 @@ mlir::Location CIRGenFunction::getLoc(SourceLocation srcLoc) { return mlir::FileLineColLoc::get(builder.getStringAttr(filename), pLoc.getLine(), pLoc.getColumn()); } - // Do our best... + // We expect to have a currSrcLoc set, so we assert here, but it isn't + // critical for the correctness of compilation, so in non-assert builds + // we fallback on using an unknown location. assert(currSrcLoc && "expected to inherit some source location"); - return *currSrcLoc; + if (currSrcLoc) + return *currSrcLoc; + // We're brave, but time to give up. + return builder.getUnknownLoc(); } mlir::Location CIRGenFunction::getLoc(SourceRange srcLoc) { @@ -128,9 +133,12 @@ mlir::Location CIRGenFunction::getLoc(SourceRange srcLoc) { mlir::Attribute metadata; return mlir::FusedLoc::get(locs, metadata, &getMLIRContext()); } - if (currSrcLoc) { + // We expect to have a currSrcLoc set, so we assert here, but it isn't + // critical for the correctness of compilation, so in non-assert builds + // we fallback on using an unknown location. + assert(currSrcLoc && "expected to inherit some source location"); + if (currSrcLoc) return *currSrcLoc; - } // We're brave, but time to give up. return builder.getUnknownLoc(); } diff --git a/clang/lib/CIR/CodeGen/CIRGenVTables.cpp b/clang/lib/CIR/CodeGen/CIRGenVTables.cpp index 1453765cc3078..00f3adfb1757f 100644 --- a/clang/lib/CIR/CodeGen/CIRGenVTables.cpp +++ b/clang/lib/CIR/CodeGen/CIRGenVTables.cpp @@ -766,8 +766,8 @@ void CIRGenFunction::generateThunk(cir::FuncOp fn, // Create lexical scope - must stay alive for entire thunk generation. // startFunction() requires currLexScope to be set. - mlir::Location unknownLoc = builder.getUnknownLoc(); - LexicalScope lexScope{*this, unknownLoc, entryBb}; + SourceLocRAIIObject locRAII(*this, fn.getLoc()); + LexicalScope lexScope{*this, fn.getLoc(), entryBb}; startThunk(fn, gd, fnInfo, isUnprototyped); assert(!cir::MissingFeatures::generateDebugInfo()); diff --git a/clang/test/CIR/CodeGen/thunks.cpp b/clang/test/CIR/CodeGen/thunks.cpp index 5a27eb373746d..15c4810738420 100644 --- a/clang/test/CIR/CodeGen/thunks.cpp +++ b/clang/test/CIR/CodeGen/thunks.cpp @@ -69,6 +69,28 @@ D::~D() {} } // namespace Test3 +namespace Test4 { +// Non-virtual this-adjusting thunk for a method with a by-value parameter. +// This exercises the case where emitDelegateCallArg is called with an invalid +// SourceLocation during thunk generation, which requires getLoc to handle +// a null currSrcLoc gracefully. + +struct A { + virtual void f(); +}; + +struct B { + virtual void g(int x); +}; + +struct C : A, B { + void g(int x) override; +}; + +void C::g(int x) {} + +} // namespace Test4 + // In CIR, all globals are emitted before functions. // Test1 vtable: C's vtable references the thunk for B's entry. @@ -81,6 +103,11 @@ D::~D() {} // CIR-DAG: #cir.global_view<@_ZN5Test21C1gEv> : !cir.ptr<!u8i> // CIR-DAG: #cir.global_view<@_ZThn8_N5Test21C1gEv> : !cir.ptr<!u8i> +// Test4 vtable: C's vtable references the thunk for B's entry. +// CIR-DAG: cir.global "private" external @_ZTVN5Test41CE = #cir.vtable<{ +// CIR-DAG: #cir.global_view<@_ZN5Test41C1gEi> : !cir.ptr<!u8i> +// CIR-DAG: #cir.global_view<@_ZThn8_N5Test41C1gEi> : !cir.ptr<!u8i> + // Test3 vtable: D's vtable references D1, D0, and their thunks. // CIR-DAG: cir.global "private" external @_ZTVN5Test31DE = #cir.vtable<{ // CIR-DAG: #cir.global_view<@_ZN5Test31DD1Ev> : !cir.ptr<!u8i> @@ -141,6 +168,21 @@ D::~D() {} // CIR: cir.call @_ZN5Test31DD0Ev(%[[T3B_RESULT]]) // CIR: cir.return +// --- Test4: by-value parameter thunk --- + +// CIR: cir.func {{.*}} @_ZN5Test41C1gEi + +// The thunk adjusts 'this' by -8 bytes, forwards the int arg, and calls C::g(). +// CIR: cir.func {{.*}} @_ZThn8_N5Test41C1gEi(%arg0: !cir.ptr< +// CIR: %[[T4_THIS:.*]] = cir.load +// CIR: %[[T4_CAST:.*]] = cir.cast bitcast %[[T4_THIS]] : !cir.ptr<{{.*}}> -> !cir.ptr<!u8i> +// CIR: %[[T4_OFFSET:.*]] = cir.const #cir.int<-8> : !s64i +// CIR: %[[T4_ADJUSTED:.*]] = cir.ptr_stride %[[T4_CAST]], %[[T4_OFFSET]] : (!cir.ptr<!u8i>, !s64i) -> !cir.ptr<!u8i> +// CIR: %[[T4_RESULT:.*]] = cir.cast bitcast %[[T4_ADJUSTED]] : !cir.ptr<!u8i> -> !cir.ptr< +// CIR: %[[T4_ARG:.*]] = cir.load +// CIR: cir.call @_ZN5Test41C1gEi(%[[T4_RESULT]], %[[T4_ARG]]) +// CIR: cir.return + // --- LLVM checks --- // LLVM: @_ZTVN5Test11CE = global { [3 x ptr], [3 x ptr] } { @@ -158,6 +200,11 @@ D::~D() {} // LLVM-SAME: [4 x ptr] [ptr inttoptr (i64 -8 to ptr), ptr null, ptr @_ZThn8_N5Test31DD1Ev, ptr @_ZThn8_N5Test31DD0Ev] // LLVM-SAME: } +// LLVM: @_ZTVN5Test41CE = global { [4 x ptr], [3 x ptr] } { +// LLVM-SAME: [4 x ptr] [ptr null, ptr null, ptr @_ZN5Test41A1fEv, ptr @_ZN5Test41C1gEi], +// LLVM-SAME: [3 x ptr] [ptr inttoptr (i64 -8 to ptr), ptr null, ptr @_ZThn8_N5Test41C1gEi] +// LLVM-SAME: } + // LLVM: define {{.*}} void @_ZThn8_N5Test11C1fEv(ptr{{.*}}) // LLVM: %[[L1_THIS:.*]] = load ptr, ptr // LLVM: %[[L1_ADJ:.*]] = getelementptr i8, ptr %[[L1_THIS]], i64 -8 @@ -178,6 +225,12 @@ D::~D() {} // LLVM: %[[L3B_ADJ:.*]] = getelementptr i8, ptr %[[L3B_THIS]], i64 -8 // LLVM: call void @_ZN5Test31DD0Ev(ptr{{.*}} %[[L3B_ADJ]]) +// LLVM: define {{.*}} void @_ZThn8_N5Test41C1gEi(ptr{{.*}}, i32{{.*}}) +// LLVM: %[[L4_THIS:.*]] = load ptr, ptr +// LLVM: %[[L4_ADJ:.*]] = getelementptr i8, ptr %[[L4_THIS]], i64 -8 +// LLVM: %[[L4_ARG:.*]] = load i32, ptr +// LLVM: call void @_ZN5Test41C1gEi(ptr{{.*}} %[[L4_ADJ]], i32{{.*}} %[[L4_ARG]]) + // --- OGCG checks --- // OGCG: @_ZTVN5Test11CE = unnamed_addr constant { [3 x ptr], [3 x ptr] } { @@ -195,6 +248,11 @@ D::~D() {} // OGCG-SAME: [4 x ptr] [ptr inttoptr (i64 -8 to ptr), ptr null, ptr @_ZThn8_N5Test31DD1Ev, ptr @_ZThn8_N5Test31DD0Ev] // OGCG-SAME: } +// OGCG: @_ZTVN5Test41CE = unnamed_addr constant { [4 x ptr], [3 x ptr] } { +// OGCG-SAME: [4 x ptr] [ptr null, ptr null, ptr @_ZN5Test41A1fEv, ptr @_ZN5Test41C1gEi], +// OGCG-SAME: [3 x ptr] [ptr inttoptr (i64 -8 to ptr), ptr null, ptr @_ZThn8_N5Test41C1gEi] +// OGCG-SAME: } + // OGCG: define {{.*}} void @_ZThn8_N5Test11C1fEv(ptr{{.*}}) // OGCG: %[[O1_THIS:.*]] = load ptr, ptr // OGCG: %[[O1_ADJ:.*]] = getelementptr inbounds i8, ptr %[[O1_THIS]], i64 -8 @@ -214,3 +272,9 @@ D::~D() {} // OGCG: %[[O3B_THIS:.*]] = load ptr, ptr // OGCG: %[[O3B_ADJ:.*]] = getelementptr inbounds i8, ptr %[[O3B_THIS]], i64 -8 // OGCG: call void @_ZN5Test31DD0Ev(ptr{{.*}} %[[O3B_ADJ]]) + +// OGCG: define {{.*}} void @_ZThn8_N5Test41C1gEi(ptr{{.*}}, i32{{.*}}) +// OGCG: %[[O4_THIS:.*]] = load ptr, ptr +// OGCG: %[[O4_ADJ:.*]] = getelementptr inbounds i8, ptr %[[O4_THIS]], i64 -8 +// OGCG: %[[O4_ARG:.*]] = load i32, ptr +// OGCG: {{.*}}call void @_ZN5Test41C1gEi(ptr{{.*}} %[[O4_ADJ]], i32{{.*}} %[[O4_ARG]]) `````````` </details> https://github.com/llvm/llvm-project/pull/185059 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
