Add test for iterating over MDNode operands when they are empty

[rastogishubham]

With e851278 the for loop that iterates over MDNode operands was changed to a range-based for loop. This change surfaces a bug where if the result of MD->operands() is an ArrayRef that has a size of 0, then iterating over that ArrayRef leads to a segmentation fault, due to accessing invalid addresses.

This was reverted with 6ce03ff but this test should be added to test that codepath in the future.

[llvmbot]

@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-debuginfo

Author: Shubham Sandeep Rastogi (rastogishubham)

Changes

With e851278 the for loop that iterates over MDNode operands was changed to a range-based for loop. This change surfaces a bug where if the result of MD->operands() is an ArrayRef that has a size of 0, then iterating over that ArrayRef leads to a segmentation fault, due to accessing invalid addresses. This patch fixes that issue.

@kazutakahirata Please let me know if this patch works.

---

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

2 Files Affected:

• (modified) llvm/lib/IR/Verifier.cpp (+5-3)
• (added) llvm/test/DebugInfo/verify-dwarf-no-operands.ll (+29)

diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index 8d992c232ca7ce..54da5b299f1c93 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -2912,9 +2912,11 @@ void Verifier::visitFunction(const Function &F) {
     for (auto &I : BB) {
       VisitDebugLoc(I, I.getDebugLoc().getAsMDNode());
       // The llvm.loop annotations also contain two DILocations.
-      if (auto MD = I.getMetadata(LLVMContext::MD_loop))
-        for (const MDOperand &MDO : llvm::drop_begin(MD->operands()))
-          VisitDebugLoc(I, dyn_cast_or_null<MDNode>(MDO));
+      if (auto MD = I.getMetadata(LLVMContext::MD_loop)) {
+        if (MD->getNumOperands())
+          for (const MDOperand &MDO : llvm::drop_begin(MD->operands()))
+            VisitDebugLoc(I, dyn_cast_or_null<MDNode>(MDO));
+      }
       if (BrokenDebugInfo)
         return;
     }
diff --git a/llvm/test/DebugInfo/verify-dwarf-no-operands.ll b/llvm/test/DebugInfo/verify-dwarf-no-operands.ll
new file mode 100644
index 00000000000000..c655289e9ee61e
--- /dev/null
+++ b/llvm/test/DebugInfo/verify-dwarf-no-operands.ll
@@ -0,0 +1,29 @@
+%"class.llvm::StringRef" = type { ptr, i64 }
+define internal void @_ZL30tokenizeWindowsCommandLineImplN4llvm9StringRefERNS_11StringSaverENS_12function_refIFvS0_EEEbNS3_IFvvEEEb() #0 !dbg !12 {
+  %7 = alloca %"class.llvm::StringRef", align 8
+  %21 = call noundef i64 @_ZNK4llvm9StringRef4sizeEv(ptr noundef nonnull align 8 dereferenceable(16) %7), !dbg !264
+  br label %22, !dbg !265
+  br label %22, !llvm.loop !284
+}
+define linkonce_odr noundef i64 @_ZNK4llvm9StringRef4sizeEv() #0 align 2 !dbg !340 {
+  %2 = alloca ptr, align 8
+  %3 = load ptr, ptr %2, align 8
+  %4 = getelementptr inbounds %"class.llvm::StringRef", ptr %3, !dbg !344
+  %5 = load i64, ptr %4, !dbg !344
+  ret i64 %5, !dbg !345
+}
+!llvm.module.flags = !{!2, !6}
+!llvm.dbg.cu = !{!7}
+!2 = !{i32 2, !"Debug Info Version", i32 3}
+!6 = !{i32 7, !"frame-pointer", i32 1}
+!7 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !8, sdk: "MacOSX14.0.sdk")
+!8 = !DIFile(filename: "file.cpp", directory: "/Users/Dev", checksumkind: CSK_MD5, checksum: "ed7ae158f20f7914bc5fb843291e80da")
+!12 = distinct !DISubprogram(unit: !7, retainedNodes: !36)
+!36 = !{}
+!260 = distinct !DILexicalBlock(scope: !12, line: 412, column: 3)
+!264 = !DILocation(scope: !260)
+!265 = !DILocation(scope: !260, column: 20)
+!284 = distinct !{}
+!340 = distinct !DISubprogram(unit: !7, retainedNodes: !36)
+!344 = !DILocation(scope: !340)
+!345 = !DILocation(scope: !340)

[adrian-prantl]

This will not work in a build without a backend. You should probably move this with the other verifier tests in test/Verifier and just run llvm-as on it.

[adrian-prantl]

Can you also comment which line is the interesting one?

[adrian-prantl]

@rastogishubham isn't there another change in the same commit at line 4720 that has the exact same problem?

[adrian-prantl]

actually it looks like there are multiple places that make the drop_begin mistake. Which makes me wonder if drop_begin should just return an empty range in the failure case?

[adrian-prantl]

https://llvm.org/doxygen/STLExtras_8h_source.html#l00329
looks like it definitely doesn't.
@kazutakahirata Can you take care of the other occurrences in your patch?

[adrian-prantl]

@kazutakahirata In hindsight your original patch would have benefitted from a pre-commit review.
@rastogishubham Since there's more than one problem with it, I think it's best if you just revert e851278 but still update and land your test since it showcases a code path that previously wasn't covered.

[felipepiovezan]

@kazutakahirata In hindsight your original patch would have benefitted from a pre-commit review. @rastogishubham Since there's more than one problem with it, I think it's best if you just revert e851278 but still update and land your test since it showcases a code path that previously wasn't covered.

Hi @kazutakahirata, it might be worth checking some of your other commits as well, I've seen at least one other that should probably be reverted for the same reason: 7d269a4

Also wanted to take this opportunity to raise something related: I've noticed that the vast majority of your commits in the four months were NFC patches that were not previously posted for review. I'd really appreciate if subsequent patches like this went through the normal review process first, for a number of reasons:

The developer policy is pretty clear about what can be committed without review:

You are allowed to commit patches without approval which you think are obvious. This is clearly a subjective decision — we simply expect you to use good judgement. Examples include: fixing build breakage, reverting obviously broken patches, documentation/comment changes, any other minor changes.

Most of your patches would be candidates for the "other minor changes" category, which is a bit tricky to evaluate. As this bug proves, even the best intentioned NFC change can break things, and reviewers would have caught this.

The policy also states that:

Avoid committing formatting- or whitespace-only changes outside of code you plan to make subsequent changes to

In most cases, I believe you were not intending to make subsequent changes to that same piece of the codebase (based on the git log).

Also, in general, the community is pretty sensible to large-scale refactors for a number of reasons that have been discussed in the past. While none of your patches are large-scale, their collective certainly is.

[dwblaikie]

Also wanted to take this opportunity to raise something related: I've noticed that the vast majority of your commits in the four months were NFC patches that were not previously posted for review. I'd really appreciate if subsequent patches like this went through the normal review process first, for a number of reasons:

The developer policy is pretty clear about what can be committed without review:

You are allowed to commit patches without approval which you think are obvious. This is clearly a subjective decision — we simply expect you to use good judgement. Examples include: fixing build breakage, reverting obviously broken patches, documentation/comment changes, any other minor changes.

Most of your patches would be candidates for the "other minor changes" category, which is a bit tricky to evaluate. As this bug proves, even the best intentioned NFC change can break things, and reviewers would have caught this.

I think these patches do qualify as not requiring precommit review according to policy - doesn't mean they can't be sent for review, might not need more care in the future (with or without precommit review) - but I don't think these commits were /against/ policy.

In general @kazutakahirata's commits, especially earlier on when they started on this sort of clang-tidy-like cleanups were precommit reviewed a lot - at some point the cost/benefit of that didn't seem worth it anymore, and most of the patches have been post-commit reviewed.

Personally, I'm OK with that - yes, some might require more care/fine tuning about how carefully to check the code/semantics of the replacement, but I think this is within the sort of thing that post-commit review/testing is suitable for.

If anything, maybe some place on the forums to post "does anyone see anything wrong with this general class of tidy/fixit/change (not each specific instance), if not, I'm going to apply this across the LLVM repo" for each type of change, perhaps.

The policy also states that:

Avoid committing formatting- or whitespace-only changes outside of code you plan to make subsequent changes to

In most cases, I believe you were not intending to make subsequent changes to that same piece of the codebase (based on the git log).

These aren't formatting or whitespace-only changes, though. So I don't think this applies.

[felipepiovezan]

If anything, maybe some place on the forums to post "does anyone see anything wrong with this general class of tidy/fixit/change (not each specific instance), if not, I'm going to apply this across the LLVM repo" for each type of change, perhaps.

This is the key here IMO. If we had taken this to the forums, I suspect some of these classes of changes would have seen pushback, either in a more general form or from subprojects.
Historically, we have been pretty averse to widespread changes like those, we favor doing these changes at the same time as the same code lines are being moved or changed for other reasons. To cite some examples: we've tried to change the camel case convention in one sweep, we've tried to perform clang-format in a similar fashion, these were all shut off.

I think these patches do qualify as not requiring precommit review according to policy

That's another issue here, it's pretty difficult to tell. A handful of those are probably fine, but if we count how many such changes were pushed last year, the volume is a bit high: 623 commits with the "NFC" tag since Jan 1 2023. Considering this volume of commits is coming from a single author, it's pretty difficult as a group to evaluate what's going on, and the cost of bugs like this can be high.

These aren't formatting or whitespace-only changes, though. So I don't think this applies.

I understand your point if take the policy literally, but IMO the spirit of that guideline is to avoid polluting the git log and avoid some downstream churn. Maybe we should try to clarify if this is the intent or not.

[dwblaikie]

If anything, maybe some place on the forums to post "does anyone see anything wrong with this general class of tidy/fixit/change (not each specific instance), if not, I'm going to apply this across the LLVM repo" for each type of change, perhaps.

This is the key here IMO. If we had taken this to the forums, I suspect some of these classes of changes would have seen pushback, either in a more general form or from subprojects. Historically, we have been pretty averse to widespread changes like those, we favor doing these changes at the same time as the same code lines are being moved or changed for other reasons. To cite some examples: we've tried to change the camel case convention in one sweep, we've tried to perform clang-format in a similar fashion, these were all shut off.

My experience has been pretty different - naming conventions and and formatting have generally been disfavored, yes - but refactorings like the ones we're discussing haven't, in my experience, ever received much, if any, pushback. Some of that happening in broader refactorings, some more ad-hoc. Some of this NFC work has even included things like making APIs match so that migrations to standard features are easier - such as the migration from llvm::Optional to std::optional.

I think these patches do qualify as not requiring precommit review according to policy

That's another issue here, it's pretty difficult to tell. A handful of those are probably fine, but if we count how many such changes were pushed last year, the volume is a bit high: 623 commits with the "NFC" tag since Jan 1 2023. Considering this volume of commits is coming from a single author, it's pretty difficult as a group to evaluate what's going on, and the cost of bugs like this can be high.

What sort of costs do you have in mind? I think these changes have been, on average, lower cost than most (that these sort of changes are generally pretty safe) - even this one seems like it was picked up pretty quickly, blamed appropriately, etc.

These aren't formatting or whitespace-only changes, though. So I don't think this applies.

I understand your point if take the policy literally, but IMO the spirit of that guideline is to avoid polluting the git log and avoid some downstream churn. Maybe we should try to clarify if this is the intent or not.

Those are the motivations, yes, weighed against the value of such name/whitespace changes - I think in the case of these sort of API refactorings/simplifications, the cost/benefit tips in favor of the changes moreso than name/whitespace changes (especially since these sort of refactorings have value on a per-application basis moreso than naming/whitespace where the benefit is mostly gained by pervasive changes & so there's more need to get buy-in to make such pervasive changes) - that these improve readability in ways that are easier to justify than pervasive naming/whitespace changes.

Like I said - generally these sort of changes were sent for review, a lot, early on, and at some point the feedback was so infrequent that it didn't seem worthwhile anymore.