Skip to content

Conversation

@laxmansole
Copy link
Contributor

LLVM IR verifier checks for extraData in debug info metadata.

This is a follow-up PR based on discussions in #165023

@llvmbot
Copy link
Member

llvmbot commented Nov 13, 2025

@llvm/pr-subscribers-llvm-ir

Author: Laxman Sole (laxmansole)

Changes

LLVM IR verifier checks for extraData in debug info metadata.

This is a follow-up PR based on discussions in #165023


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

1 Files Affected:

  • (modified) llvm/lib/IR/Verifier.cpp (+17)
diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index fa18c3cd0f404..b01ca73aa3623 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -1320,6 +1320,23 @@ void Verifier::visitDIDerivedType(const DIDerivedType &N) {
   if (N.getTag() == dwarf::DW_TAG_ptr_to_member_type) {
     CheckDI(isType(N.getRawExtraData()), "invalid pointer to member type", &N,
             N.getRawExtraData());
+  } else if (N.getTag() == dwarf::DW_TAG_template_alias) {
+    CheckDI(isa<MDTuple>(N.getRawExtraData()), "invalid template parameters",
+            &N, N.getRawExtraData());
+  } else if (auto *ExtraData = N.getRawExtraData()) {
+    auto IsValidExtraData = [&]() {
+      if (isa<ConstantAsMetadata>(ExtraData) || isa<MDString>(ExtraData) ||
+          isa<DIObjCProperty>(ExtraData))
+        return true;
+      if (auto *Tuple = dyn_cast<MDTuple>(ExtraData))
+        return Tuple->getNumOperands() == 1 &&
+               isa<ConstantAsMetadata>(Tuple->getOperand(0));
+      return false;
+    };
+    CheckDI(IsValidExtraData(),
+            "extraData must be ConstantAsMetadata, MDString, DIObjCProperty, "
+            "or MDTuple with single ConstantAsMetadata operand",
+            &N, ExtraData);
   }
 
   if (N.getTag() == dwarf::DW_TAG_set_type) {

@laxmansole
Copy link
Contributor Author

@ayermolo ayermolo requested a review from Copilot November 13, 2025 23:02
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds LLVM IR verifier checks for the extraData field in DIDerivedType debug info metadata. The changes ensure that extraData contains only valid metadata types based on the debug info tag, preventing invalid metadata from being created.

Key Changes:

  • Added validation for DW_TAG_template_alias to ensure extraData is an MDTuple
  • Added general validation for all other tags with extraData to restrict it to specific allowed metadata types

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 1327 to 1343
auto IsValidExtraData = [&]() {
if (isa<ConstantAsMetadata>(ExtraData) || isa<MDString>(ExtraData) ||
isa<DIObjCProperty>(ExtraData))
return true;
if (auto *Tuple = dyn_cast<MDTuple>(ExtraData))
return Tuple->getNumOperands() == 1 &&
isa<ConstantAsMetadata>(Tuple->getOperand(0));
return false;
};
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

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

The lambda does not validate that an MDTuple with a single operand actually contains a ConstantAsMetadata. If getOperand(0) returns null, the check will pass but may lead to undefined behavior. Add a null check before calling isa<ConstantAsMetadata>.

Copilot generated this review using guidance from repository custom instructions.
@ayermolo
Copy link
Contributor

Test?

@ayermolo ayermolo self-requested a review November 13, 2025 23:04
@dzhidzhoev dzhidzhoev self-requested a review November 14, 2025 16:52
CheckDI(isType(N.getRawExtraData()), "invalid pointer to member type", &N,
N.getRawExtraData());
} else if (N.getTag() == dwarf::DW_TAG_template_alias) {
CheckDI(isa<MDTuple>(N.getRawExtraData()), "invalid template parameters",
Copy link
Member

@dzhidzhoev dzhidzhoev Nov 14, 2025

Choose a reason for hiding this comment

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

We shouldn't crash if N.getRawExtraData() returns nullptr. (also relevant for the fragment reviewed by Copilot)

if (isa<ConstantAsMetadata>(ExtraData) || isa<MDString>(ExtraData) ||
isa<DIObjCProperty>(ExtraData))
return true;
if (auto *Tuple = dyn_cast<MDTuple>(ExtraData))
Copy link
Member

Choose a reason for hiding this comment

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

Should we allow this only for DW_TAG_member, DW_TAG_variable, and DW_TAG_inheritance tags?

@laxmansole laxmansole force-pushed the user/lsole/extraData_verifier_checks branch from 1423cbb to f583bc3 Compare November 15, 2025 03:15
Copy link
Member

@dzhidzhoev dzhidzhoev left a comment

Choose a reason for hiding this comment

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

From what I see in pre-merge check results, some tests fail with this patch. Probably, we should limit the scope of extraData check to specific tags (if the reason of failures is that the checks in this patch apply to tags other than DW_TAG_member, DW_TAG_variable, or DW_TAG_inheritance — the only ones that have MDTuple-in-extraData support implemented. Supporting extraData check for all possible tags is nice, but I don't insist on that if it gets too complicated, as it is not relevant to #165023).

@laxmansole laxmansole force-pushed the user/lsole/extraData_verifier_checks branch from f583bc3 to bff5946 Compare November 17, 2025 20:00
@github-actions
Copy link

🐧 Linux x64 Test Results

  • 186262 tests passed
  • 4852 tests skipped

Copy link
Member

@dzhidzhoev dzhidzhoev left a comment

Choose a reason for hiding this comment

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

LGTM

I think it's better to use [IR] tag instead of [LLVM IR] in the commit title, as it is more widely used if we look at git log.

@laxmansole laxmansole changed the title [DebugInfo][LLVM IR] Verifier checks for the extraData [DebugInfo][IR] Verifier checks for the extraData Nov 18, 2025
@enferex enferex merged commit 58b8e6e into llvm:main Nov 18, 2025
10 checks passed
@laxmansole laxmansole deleted the user/lsole/extraData_verifier_checks branch November 18, 2025 21:50
aadeshps-mcw pushed a commit to aadeshps-mcw/llvm-project that referenced this pull request Nov 26, 2025
LLVM IR verifier checks for `extraData` in debug info metadata. 

This is a follow-up PR based on discussions in llvm#165023
Priyanshu3820 pushed a commit to Priyanshu3820/llvm-project that referenced this pull request Nov 26, 2025
LLVM IR verifier checks for `extraData` in debug info metadata. 

This is a follow-up PR based on discussions in llvm#165023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants