Skip to content
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

[RemoveDIs] Add additional debug-mode verifier checks #84308

Merged
merged 3 commits into from
Mar 11, 2024

Conversation

OCHyams
Copy link
Contributor

@OCHyams OCHyams commented Mar 7, 2024

Separated from #83251

@llvmbot
Copy link
Member

llvmbot commented Mar 7, 2024

@llvm/pr-subscribers-llvm-ir

Author: Orlando Cazalet-Hyams (OCHyams)

Changes

Separated from #83251


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

1 Files Affected:

  • (modified) llvm/lib/IR/Verifier.cpp (+10)
diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index e0de179e57146f..e6e617cc40cb2d 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -2674,6 +2674,11 @@ void Verifier::visitFunction(const Function &F) {
   Check(verifyAttributeCount(Attrs, FT->getNumParams()),
         "Attribute after last parameter!", &F);
 
+  Check(F.IsNewDbgInfoFormat == F.getParent()->IsNewDbgInfoFormat,
+        "Fn debug format should match parent", &F,
+        F.IsNewDbgInfoFormat, F.getParent(),
+        F.getParent()->IsNewDbgInfoFormat);
+
   bool IsIntrinsic = F.isIntrinsic();
 
   // Check function attributes.
@@ -3017,6 +3022,11 @@ void Verifier::visitBasicBlock(BasicBlock &BB) {
     Check(I.getParent() == &BB, "Instruction has bogus parent pointer!");
   }
 
+  Check(BB.IsNewDbgInfoFormat == BB.getParent()->IsNewDbgInfoFormat,
+        "BB debug format should match parent", &BB,
+        BB.IsNewDbgInfoFormat, BB.getParent(),
+        BB.getParent()->IsNewDbgInfoFormat);
+
   // Confirm that no issues arise from the debug program.
   if (BB.IsNewDbgInfoFormat)
     CheckDI(!BB.getTrailingDPValues(), "Basic Block has trailing DbgRecords!",

Copy link

github-actions bot commented Mar 7, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Contributor

@SLTozer SLTozer left a comment

Choose a reason for hiding this comment

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

Should this be Check, or CheckDI? On the one hand this is a check that shouldn't fail due to a bad input, but only due to an error in LLVM itself; on the other hand, if it could be resolved by stripping debug info then it might be better to allow that in case someone encounters this in the wild? I'm not strongly opinionated either way, since there's precedent for internal debug info errors being a verifier failure.

@@ -2674,6 +2674,11 @@ void Verifier::visitFunction(const Function &F) {
Check(verifyAttributeCount(Attrs, FT->getNumParams()),
"Attribute after last parameter!", &F);

Check(F.IsNewDbgInfoFormat == F.getParent()->IsNewDbgInfoFormat,
"Fn debug format should match parent", &F,

Choose a reason for hiding this comment

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

May be instead of "Fn ..." use "F ..." or "Function ...".

@OCHyams OCHyams merged commit a84eb24 into llvm:main Mar 11, 2024
3 of 4 checks passed
OCHyams added a commit that referenced this pull request Mar 11, 2024
OCHyams added a commit that referenced this pull request Mar 11, 2024
…at-pointers

Fixes failing tests after #84308

LLVM :: CodeGen/AMDGPU/GlobalISel/irtranslator-non-integral-address-spaces-vectors.ll
LLVM :: CodeGen/AMDGPU/GlobalISel/irtranslator-non-integral-address-spaces.ll
LLVM :: CodeGen/AMDGPU/lower-buffer-fat-pointers-calls.ll
LLVM :: CodeGen/AMDGPU/lower-buffer-fat-pointers-constants.ll
LLVM :: CodeGen/AMDGPU/lower-buffer-fat-pointers-pointer-ops.ll
LLVM :: CodeGen/AMDGPU/pal-metadata-3.0.ll

Buildbots: https://lab.llvm.org/buildbot/#/builders/121/builds/39855
@OCHyams
Copy link
Contributor Author

OCHyams commented Mar 12, 2024

This relanded in 2953d9c

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.

4 participants