From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: dyndbg: fix NULL ptr on i386 due to section alignment Date: Sat, 14 Mar 2026 07:02:58 +1000 Message-ID: In-Reply-To: <20260313131045.2526422-2-jim.cromie@gmail.com> References: <20260313131045.2526422-1-jim.cromie@gmail.com> <20260313131045.2526422-2-jim.cromie@gmail.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Commit message quality:** Good. Clearly explains the root cause (28-byte = `__dyndbg_classes` structs on i386 causing misalignment of following `__dyn= dbg` section) and the three-pronged fix. **Review of the changes:** 1. **ALIGN(8) added to BOUNDED_SECTION macros** (lines 492, 498 of the diff= ): ```c #define BOUNDED_SECTION_PRE_LABEL(_sec_, _label_, _BEGIN_, _END_) \ . =3D ALIGN(8); \ _BEGIN_##_label_ =3D .; \ ``` This is a reasonable belt-and-suspenders approach. However, this is a **bro= adly impactful change** =E2=80=94 `BOUNDED_SECTION_PRE_LABEL` and `BOUNDED_= SECTION_POST_LABEL` are used by many sections beyond dyndbg (e.g., `__bug_t= able`, jump tables, etc.). Adding ALIGN(8) to *all* bounded sections to fix= a dyndbg-specific issue is heavy-handed. It may introduce unnecessary padd= ing in sections that have smaller natural alignment requirements. The commi= t message acknowledges this for ORC but doesn't discuss the impact on other= users of the macro. **Concern:** Has the impact on vmlinux size been measured across archite= ctures? Adding ALIGN(8) padding before every bounded section could bloat th= e binary. The arch maintainers (especially Arnd Bergmann, who is CC'd) shou= ld weigh in. 2. **Section reordering** (lines 506-508): ```c BOUNDED_SECTION_BY(__dyndbg, ___dyndbg) \ BOUNDED_SECTION_BY(__dyndbg_classes, ___dyndbg_classes) \ ``` Moving `__dyndbg` before `__dyndbg_classes` so it sits directly after the e= xisting `ALIGN(8)` is the minimal correct fix. This alone would solve the i= mmediate problem without the broader macro change. 3. **ORC unwinder de-macroification** (lines 516-533): ```c __start_orc_header =3D .; \ KEEP(*(.orc_header)) \ __stop_orc_header =3D .; \ ``` Manually expanding the macro for ORC sections to avoid the new ALIGN(8) is = correct =E2=80=94 ORC has ALIGN(4) and ALIGN(2) requirements, and `scripts/= sorttable.c` cannot tolerate extra padding. However, this creates a mainten= ance concern: these three sections now duplicate what `BOUNDED_SECTION_BY` = would generate (minus the alignment). If `BOUNDED_SECTION_BY` changes seman= tics in the future, ORC won't track those changes. **Suggestion:** Consider adding a comment near the ORC sections explaini= ng *why* they don't use `BOUNDED_SECTION_BY`, so future developers don't "h= elpfully" convert them back. **Nit:** The commit says "Closes:" with a lore link, which is good for trac= king. **Verdict:** The fix works but the approach of modifying a global macro is = concerning. The section reordering alone (change #2) fixes the immediate is= sue. The ALIGN(8) in the macro (change #1) is defensive but has non-trivial= blast radius. Would prefer an ack from the linker-script/arch maintainers = before merging. --- --- Generated by Claude Code Patch Reviewer