Hi Aaron,
On Tue, 2026-03-31 at 16:52 -0400, Aaron Merey wrote:
> On Thu, Mar 26, 2026 at 12:48 PM Mark Wielaard <[email protected]> wrote:
> > + /* If read as 4 bytes the version is 2, it is GNU DebugFission for
> > + DWARF 5, otherwise the version is in the first 2 bytes, with 2
> > + bytes padding. */
> > + int32_t vers = read_4ubyte_unaligned (dbg, readp);
> > + if (vers != 2)
> > + vers = read_2ubyte_unaligned (dbg, readp);
> > + fprintf (out, _(" Version: %8" PRId32 "\n"), vers);
> > +
> > + /* There used to be a version 1, which we don't support, but is
> > + described at https://gcc.gnu.org/wiki/DebugFissionDWP
> > + Version 2 is GNU DebugFission for DWARF4.
> > + Version 5 is the standardized DWARF5 index.
> > + Version 6 supports DWARF64 as described in
> > + https://dwarfstd.org/issues/220708.2.html */
> > + if (vers != 2 && vers != 5 && vers != 6)
> > + {
> > + error (0, 0, _("unknown version, cannot parse section"));
> > + return;
> > + }
> > +
> > + uint8_t offset_size_flag = 0;
> > + if (vers < 2)
>
> vers < 2 is always false here since there's an early return above if
> vers isn't 2, 5 or 6.
>
> > + readp += 4;
> > + else
> > + {
> > + readp += 3;
> > + offset_size_flag = *readp;
> > + fprintf (out, _(" Offset Size: %4" PRIu8 "\n"),
> > + offset_size_flag == 0 ? 32 : 64);
> > + readp++;
> > + }
Oops. Indeed that should be if (vers < 6). Only version 6 has the
Offset size field. Fixed locally. It only worked by accident, because
before version 6 it is followed by 4 bytes (zero) padding. Which makes
the offset_size_flag 32 (as expected for version 2). It does mean the
testfile output had an extra " Offset Size: 32" line. Also removed
locally.
> > + for (size_t i = 0; i < slot_count; i++)
> > + {
> > + uint64_t id;
> > + uint32_t row;
> > + id = read_8ubyte_unaligned (dbg, hash_table + i * 8);
> > + row = read_4ubyte_unaligned (dbg, indices + i * 4);
> > + /* Only print used rows. */
> > + if (id != 0 && row != 0)
> > + {
> > + fprintf (out, " [%3zd] %016" PRIx64 " ", i, id);
> > + if (row > unit_count)
> > + {
> > + error (0, 0, _("Row (%" PRIu32 ") larger than "
> > + "unit count (%" PRIu32 ")"), row, unit_count);
> > + continue;
> > + }
> > + /* Note row is one based, not zero based. */
> > + const unsigned char *prow = (offsets
> > + + ((row - 1) * section_count
> > + * off_bytes));
> > + for (size_t j = 0; j < section_count; j++)
> > + {
> > + uint32_t off = read_4ubyte_unaligned (dbg, prow + j * 4);
>
> Is the hardcoded 4 intended in j * 4 or should it use off_bytes instead?
It should use off_bytes indeed. Thanks for catching that. And it should
use read an uint64_t and print as PRIu64 if off_bytes == 8. Also fixed
locally.
> > + printf (" %6" PRIu32, off);
>
> Should be fprintf (out, ...) instead of printf. This works if
> --enable-thread-safety=no, but if =yes then helgrind warns about a
> race condition in run-readelf-cu-index.sh due to this.
We should have a buildbot with --enable-thread-safety and --enable-
helgrind --enable-valgrind-annotations.
> > + /* Note row is one based, not zero based. */
> > + const unsigned char *prow = lengths + (row - 1) * section_count *
> > 4;
> > + for (size_t j = 0; j < section_count; j++)
> > + {
> > + uint32_t off = read_4ubyte_unaligned (dbg, prow + j * 4);
> > + printf (" %6" PRIu32, off);
>
> Should be fprintf (out, ...) here too.
Fixed in both cases. Now passes make check with configure --enable-
thread-safety --enable-helgrind --enable-valgrind-annotations
Thanks for the review. I'll sent v2 with the above fixes.
Cheers,
Mark
