| Issue |
208440
|
| Summary |
[BOLT] partitionCUs emits a spurious "tries to read DIEs" DWARF warning with --update-debug-sections
|
| Labels |
BOLT
|
| Assignees |
|
| Reporter |
not4juu
|
## Description
A regression from #197859 makes `llvm-bolt --update-debug-sections` print one bogus DWARF warning per compilation unit whose abbreviation table uses `DW_FORM_ref_addr`. The rewritten output is still correct; the warning is caused by an off-by-one in the new manual DIE-tree walk in `partitionCUs()`.
## Summary
This is a BOLT regression introduced by #197859 (commit `d3ac9b5c4cab`, *[RFC][BOLT] Add a new parallel DWARF processing (2/2)*), in `bolt/lib/Rewrite/DWARFRewriter.cpp` → `partitionCUs()`. It is a deterministic logic bug, not a data race: it reproduces with the default parallel run and equally single-threaded (`-debug-thread-count=1`), because `partitionCUs()` walks each CU sequentially regardless of thread count. The problem is architecture-independent, since the faulty code walks DWARF rather than target instructions.
## Problem observation
The warning is produced purely by DWARF rewriting, so no optimization options are needed to trigger it. The minimal reproduction is:
```bash
llvm-bolt input.so -o /dev/null --update-debug-sections
```
It prints, once for every CU that has at least one abbreviation using `DW_FORM_ref_addr`:
```text
DWARF unit from offset 0x........ incl. to offset 0x........ excl. tries to read DIEs at offset 0x........
```
(the `incl.`/`excl.` offsets are the unit bounds, and the last offset equals the next unit's start offset).
`partitionCUs()` only walks the DIE tree of CUs whose abbreviation table references another unit via `DW_FORM_ref_addr`:
```cpp
if (RefAddrAbbrevs.empty())
continue; // CUs without cross-CU ref_addr are skipped entirely
```
so the warning appears once per such CU.
## Root cause
`partitionCUs()` walks each qualifying CU's DIE tree with a manual DFS. The `ParentIndex` stack is seeded with a `UINT32_MAX` **sentinel** to mark the root:
```cpp
SmallVector<uint32_t, 8> ParentIndex;
ParentIndex.push_back(UINT32_MAX); // root sentinel
do {
if (!DIEEntry.extractFast(*CU, &DIEOffset, DebugInfoData,
NextCUOffset, ParentIndex.back()))
break;
// ...
if (Abbrev) {
// ...
if (Abbrev->hasChildren())
ParentIndex.push_back(0);
} else {
ParentIndex.pop_back(); // pops a null-DIE level
}
} while (!ParentIndex.empty()); // <-- sentinel is never popped
```
The sentinel is **never popped** (there is exactly one null DIE per level with children, and none for the root sentinel itself). After the last top-level DIE's null terminator is consumed:
- `DIEOffset` already equals `NextCUOffset`, **but**
- `ParentIndex` is not empty (it still holds `UINT32_MAX`),
so the loop runs **one extra iteration** and calls `extractFast()` with `DIEOffset == NextCUOffset`. That out-of-bounds read is what emits the warning in `DWARFDebugInfoEntry::extractFast`:
```cpp
// llvm/lib/DebugInfo/DWARF/DWARFDebugInfoEntry.cpp
if (Offset >= UEndOffset) {
U.getContext().getWarningHandler()(createStringError(
errc::invalid_argument,
"DWARF unit from offset 0x%8.8" PRIx64 " incl. to offset 0x%8.8" PRIx64
" excl. tries to read DIEs at offset 0x%8.8" PRIx64,
U.getOffset(), U.getNextUnitOffset(), *OffsetPtr));
return false;
}
```
The cross-reference result is unaffected — the extra `extractFast()` returns `false` and the loop `break`s after all real DIEs have already been processed — so the only visible effect is the spurious warning.
## Proposed fix
Terminate the loop once the CU has been fully traversed, before another read is attempted:
```diff
diff --git a/bolt/lib/Rewrite/DWARFRewriter.cpp b/bolt/lib/Rewrite/DWARFRewriter.cpp
--- a/bolt/lib/Rewrite/DWARFRewriter.cpp
+++ b/bolt/lib/Rewrite/DWARFRewriter.cpp
@@ static SmallVector<SmallVector<DWARFUnit *>> partitionCUs(DWARFContext &DwCtx,
} else {
ParentIndex.pop_back();
}
- } while (!ParentIndex.empty());
+ } while (DIEOffset < NextCUOffset && !ParentIndex.empty());
}
```
Because `DIEOffset` reaches `NextCUOffset` only after the whole unit has been consumed, this cannot drop a legitimate DIE. `extractFast()` already bounds-checks the offset, so this change does not add safety — it simply avoids the benign out-of-bounds call that produces the warning.
> Note that with the sentinel design `ParentIndex` never actually becomes empty, so `!ParentIndex.empty()` is effectively dead. An equally correct and slightly cleaner alternative is to make the offset bound the sole condition:
```cpp
} while (DIEOffset < NextCUOffset);
```
Maintainers may also prefer **not to seed the sentinel at all**, or to loop while `ParentIndex.size() > 1`; all of these are semantically equivalent.
## Environment
- **llvm-bolt:** built from `main` (reproduced on `4405d6e56d43` and `f05387745e53`)
- **Verified on:** aarch64 (architecture-independent can be reproduced at x86_64)
- **Compiler**: g++ (GCC) 14.2.0
_Follow-up fix for #197859 — cc @Thrrreeee_
![]()
_______________________________________________
llvm-bugs mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-bugs
