Skip to content

[KeyInstr][Clang] Catch variable init atom #134641

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

Merged
merged 3 commits into from
May 27, 2025

Conversation

OCHyams
Copy link
Contributor

@OCHyams OCHyams commented Apr 7, 2025

This patch is part of a stack that teaches Clang to generate Key Instructions
metadata for C and C++.

The Key Instructions project is introduced, including a "quick summary" section
at the top which adds context for this PR, here:
https://discourse.llvm.org/t/rfc-improving-is-stmt-placement-for-better-interactive-debugging/82668

The feature is only functional in LLVM if LLVM is built with CMake flag
LLVM_EXPERIMENTAL_KEY_INSTRUCTIONs. Eventually that flag will be removed.

The Clang-side work is demoed here:
#130943

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:codegen IR generation bugs: mangling, exceptions, etc. labels Apr 7, 2025
Copy link
Contributor Author

OCHyams commented Apr 7, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

@llvmbot
Copy link
Member

llvmbot commented Apr 7, 2025

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: Orlando Cazalet-Hyams (OCHyams)

Changes

This patch is part of a stack that teaches Clang to generate Key Instructions
metadata for C and C++.

The Key Instructions project is introduced, including a "quick summary" section
at the top which adds context for this PR, here:
https://discourse.llvm.org/t/rfc-improving-is-stmt-placement-for-better-interactive-debugging/82668

The feature is only functional in LLVM if LLVM is built with CMake flag
LLVM_EXPERIMENTAL_KEY_INSTRUCTIONs. Eventually that flag will be removed.

The Clang-side work is demoed here:
#130943


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

2 Files Affected:

  • (modified) clang/lib/CodeGen/ItaniumCXXABI.cpp (+1)
  • (added) clang/test/KeyInstructions/try-catch.cpp (+20)
diff --git a/clang/lib/CodeGen/ItaniumCXXABI.cpp b/clang/lib/CodeGen/ItaniumCXXABI.cpp
index 77e995b4c933a..b3f1e208fa318 100644
--- a/clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ b/clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -5051,6 +5051,7 @@ void ItaniumCXXABI::emitBeginCatch(CodeGenFunction &CGF,
 
   // Emit the local.
   CodeGenFunction::AutoVarEmission var = CGF.EmitAutoVarAlloca(*CatchParam);
+  ApplyAtomGroup Grp(CGF.getDebugInfo());
   InitCatchParam(CGF, *CatchParam, var.getObjectAddress(CGF), S->getBeginLoc());
   CGF.EmitAutoVarCleanups(var);
 }
diff --git a/clang/test/KeyInstructions/try-catch.cpp b/clang/test/KeyInstructions/try-catch.cpp
new file mode 100644
index 0000000000000..3d1080aca2f07
--- /dev/null
+++ b/clang/test/KeyInstructions/try-catch.cpp
@@ -0,0 +1,20 @@
+// RUN: %clang -gkey-instructions %s -gmlt -S -emit-llvm -o - -fexceptions \
+// RUN: | FileCheck %s
+
+void except() {
+  // FIXME(OCH): Should `store i32 32, ptr %exception` be key?
+  throw 32;
+}
+
+void attempt() {
+  try { except(); }
+// CHECK: catch:
+// CHECK: %4 = call ptr @__cxa_begin_catch(ptr %exn)
+// CHECK: %5 = load i32{{.*}}, !dbg [[G1R2:!.*]]
+// CHECK: store i32 %5, ptr %e{{.*}}, !dbg [[G1R1:!.*]]
+// CHECK: call void @__cxa_end_catch()
+  catch (int e) { }
+}
+
+// CHECK: [[G1R2]] = !DILocation({{.*}}, atomGroup: 1, atomRank: 2)
+// CHECK: [[G1R1]] = !DILocation({{.*}}, atomGroup: 1, atomRank: 1)

@OCHyams OCHyams force-pushed the users/OCHyams/ki-clang-member-init branch from c25359b to 58f7f4d Compare May 21, 2025 14:25
@OCHyams OCHyams force-pushed the users/OCHyams/ki-clang-catch-init branch from 638b319 to 54eca71 Compare May 21, 2025 14:30
Copy link
Member

@jmorse jmorse left a comment

Choose a reason for hiding this comment

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

Looking good, with a question inline.

@@ -5055,6 +5055,7 @@ void ItaniumCXXABI::emitBeginCatch(CodeGenFunction &CGF,

// Emit the local.
CodeGenFunction::AutoVarEmission var = CGF.EmitAutoVarAlloca(*CatchParam);
ApplyAtomGroup Grp(CGF.getDebugInfo());
InitCatchParam(CGF, *CatchParam, var.getObjectAddress(CGF), S->getBeginLoc());
CGF.EmitAutoVarCleanups(var);
Copy link
Member

Choose a reason for hiding this comment

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

Risk that a destructor for the catch-parameter could be given this instruction group?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes if a few things were different and wrong. 1. Calls aren't annotated. 2. If they were, it would be a bug if whatever call handling code didn't have its own ApplyAtomGroup for the dtor.

I could add a scope around ApplyAtomGroup and InitCatchParam, and/or add more tests, if you think it would be useful?

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm -- yes please for the braces, no worries about the tests. This is just about reducing the amount of potentially-unexpected behaviour

@OCHyams OCHyams force-pushed the users/OCHyams/ki-clang-member-init branch from 58f7f4d to 5d45280 Compare May 23, 2025 08:41
Base automatically changed from users/OCHyams/ki-clang-member-init to main May 23, 2025 09:02
@OCHyams OCHyams marked this pull request as draft May 23, 2025 12:00
OCHyams added 2 commits May 23, 2025 13:01
This patch is part of a stack that teaches Clang to generate Key Instructions
metadata for C and C++.

The Key Instructions project is introduced, including a "quick summary" section
at the top which adds context for this PR, here:
https://discourse.llvm.org/t/rfc-improving-is-stmt-placement-for-better-interactive-debugging/82668

The feature is only functional in LLVM if LLVM is built with CMake flag
LLVM_EXPERIMENTAL_KEY_INSTRUCTIONs. Eventually that flag will be removed.

The Clang-side work is demoed here:
#130943
@OCHyams OCHyams force-pushed the users/OCHyams/ki-clang-catch-init branch from 46e356a to 5d308dd Compare May 23, 2025 12:01
@OCHyams OCHyams marked this pull request as ready for review May 23, 2025 12:17
Copy link
Member

@jmorse jmorse left a comment

Choose a reason for hiding this comment

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

(LGTM with the braces)

@OCHyams OCHyams merged commit bf1d422 into main May 27, 2025
6 of 10 checks passed
@OCHyams OCHyams deleted the users/OCHyams/ki-clang-catch-init branch May 27, 2025 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:codegen IR generation bugs: mangling, exceptions, etc. clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants