Skip to content

[CIR] Correctly handle annotate attribute on C++ constructors/destructors #1699

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Sharp0802
Copy link

This PR fixes an issue where the [[clang::annotate]] attribute on C++ constructors (CXXConstructorDecl) was not correctly lowered, and resolves a subsequent crash in the cir-to-llvm pipeline caused by the initial fix.

1. The Problem

There were two distinct problems:

  • Initial Bug: The [[clang::annotate]] attribute was completely ignored when applied to a C++ constructor. While it worked for regular functions, the specific code path for handling CXXConstructorDecl in CIRGenModule did not process this attribute.

  • Downstream Crash: After fixing the initial bug to generate the correct annotations and cir.global_annotations in the CIR dialect, the cir-translate tool would crash with a "redefinition of symbol" error for the annotation string (e.g., .str.annotation).

2. Analysis and Root Cause

  • Cause of Initial Bug: In CIRGenModule::emitGlobalDefinition, the code path for constructors and destructors branches to ABI->emitCXXStructor. This path was missing the logic to check for an AnnotateAttr and add it to the deferredAnnotations map, which is correctly handled for regular FunctionDecls.

  • Cause of Downstream Crash: The cir-to-llvm lowering pipeline has two mechanisms for handling annotations:

    1. The LoweringPrepare pass processes the cir.global_annotations attribute on the ModuleOp and generates the corresponding @llvm.global.annotations array and its associated global string constants.
    2. A later stage in the cir-translate binary also attempts to process the same cir.global_annotations attribute.

    The issue was that LoweringPreparePass did not consume (remove) the cir.global_annotations attribute after processing it. This left the attribute on the module, causing the later stage in cir-translate to re-process it, leading to the symbol redefinition crash.

3. Implementation

This PR addresses both issues with two key changes:

  1. In CIRGenModule.cpp:

    • The logic to handle AnnotateAttr has been added to the CXXConstructorDecl / CXXDestructorDecl path within emitGlobalDefinition. This ensures that constructor annotations are correctly identified and deferred for processing, just like regular functions.
  2. In LoweringPreparePass:

    • After the buildGlobalAnnotationValues() function successfully processes the cir.global_annotations attribute and generates the necessary LLVM globals, the pass now removes the cir.global_annotations attribute from the ModuleOp. This "consumes" the attribute, preventing any subsequent pass from redundantly processing it.

4. Verification

With these changes, a C++ constructor with a [[clang::annotate]] attribute is now correctly lowered through the entire pipeline:

  • The ClangIR (cir dialect) correctly contains both the local annotations on the cir.func and the module-level cir.global_annotations.
  • The cir-opt -cir-to-llvm pass successfully lowers this to the LLVM dialect.
  • The cir-translate tool successfully converts the LLVM dialect to LLVM IR text without crashing.
  • The final LLVM IR contains the expected @llvm.global.annotations metadata for the constructor.

This fix ensures that annotation metadata is preserved correctly and robustly for C++ constructors.

Copy link

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff 32e3971d31452620eb102f122259b429733b3ef0 67e6ac3525ffff8e95fdefd8d4cc0861a8234c8c --extensions cpp -- clang/lib/CIR/CodeGen/CIRGenModule.cpp clang/lib/CIR/Dialect/Transforms/LoweringPrepare.cpp clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp
View the diff from clang-format here.
diff --git a/clang/lib/CIR/Dialect/Transforms/LoweringPrepare.cpp b/clang/lib/CIR/Dialect/Transforms/LoweringPrepare.cpp
index 756416302b..069233d27b 100644
--- a/clang/lib/CIR/Dialect/Transforms/LoweringPrepare.cpp
+++ b/clang/lib/CIR/Dialect/Transforms/LoweringPrepare.cpp
@@ -1610,7 +1610,7 @@ void LoweringPreparePass::runOnOperation() {
   buildCXXGlobalInitFunc();
   buildGlobalCtorDtorList();
   buildGlobalAnnotationValues();
-  
+
   if (theModule && theModule->hasAttr("cir.global_annotations")) {
     theModule->removeAttr("cir.global_annotations");
   }

Copy link
Member

@bcardosolopes bcardosolopes left a comment

Choose a reason for hiding this comment

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

Thanks for the detailed explanation, some comments but this mostly ready to go

@@ -1610,6 +1610,10 @@ void LoweringPreparePass::runOnOperation() {
buildCXXGlobalInitFunc();
buildGlobalCtorDtorList();
buildGlobalAnnotationValues();

if (theModule && theModule->hasAttr("cir.global_annotations")) {
Copy link
Member

Choose a reason for hiding this comment

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

Remove the curly braces!

@@ -2190,6 +2190,11 @@ mlir::LogicalResult CIRToLLVMFuncOpLowering::matchAndRewrite(
cir::FuncOp op, OpAdaptor adaptor,
mlir::ConversionPatternRewriter &rewriter) const {

mlir::ModuleOp module = op->getParentOfType<mlir::ModuleOp>();
if (module->hasAttr("cir.global_annotations")) {
Copy link
Member

Choose a reason for hiding this comment

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

Same here!

@@ -2190,6 +2190,11 @@ mlir::LogicalResult CIRToLLVMFuncOpLowering::matchAndRewrite(
cir::FuncOp op, OpAdaptor adaptor,
mlir::ConversionPatternRewriter &rewriter) const {

mlir::ModuleOp module = op->getParentOfType<mlir::ModuleOp>();
if (module->hasAttr("cir.global_annotations")) {
op->removeAttr("annotations");
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary? If cir.global_annotations have been removed in lowering prepare, how come it might show up here again?

@bcardosolopes
Copy link
Member

Seems like some clang-format is needed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants