[Toybox] [PATCH] readelf: Fix the section flags handling.
enh
enh at google.com
Thu Nov 30 15:03:23 PST 2023
On Thu, Nov 30, 2023 at 2:29 PM Rob Landley <rob at landley.net> wrote:
>
> On 11/30/23 14:29, enh wrote:
> > On Thu, Nov 30, 2023 at 11:49 AM Rob Landley <rob at landley.net> wrote:
> >>
> >> On 11/30/23 13:18, enh wrote:
> >> >> Design-wise calling for an exact match for a very large block of english text is
> >> >> brittle. If spacing changes, the test breaks.
> >> >
> >> > we already have the `NOSPACE=1` "ignore whitespace differences"
> >> > functionality in the readelf tests.
> >> >
> >> > what's the actual difference in the failure you're seeing?
> >>
> >> Version skew in the host readelf:
> >>
> >> echo -ne '' | readelf -SW
> >> /home/landley/toybox/toybox/tests/files/elf/ndk-elf-note-shflags | head -32
> >> --- expected 2023-11-30 13:44:43.199143754 -0600
> >> +++ actual 2023-11-30 13:44:43.203143754 -0600
> >> @@ -1,7 +1,7 @@
> >> There are 28 section headers, starting at offset 0xc74:
> >>
> >> Section Headers:
> >> - [Nr] Name Type Address Off Size ES Flg Lk Inf Al
> >> + [Nr] Name Type Addr Off Size ES Flg Lk Inf Al
> >
> > okay, that looks like the kind of nonsense you're complaining about :-(
> >
> > but why doesn't the existing test on line 34 fail this too? (or do you
> > really have a binutils older than any i've used while working on
> > toybox readelf?)
>
> I haven't made "test host" pass all the tests yet (there's been version skew
> from upgrades that _broke_ some), and I was holding off on that janitorial step
> until I upgrade my laptop to current debian.
>
> I applied the patch. Doesn't make the situation _worse_. But in general the
> approach of matching a large block of text exactly, some of which is clearly a
> matter of opinion, makes me nervous and I'd like to clean it up in future.
>
> Which is design work, I think I'm missing some test plumbing. I could just pipe
> to the output through sed/grep again, but there should be an easier syntax.
>
> >> [ 0] NULL 00000000 000000 000000 00 0 0 0
> >> [ 1] .interp PROGBITS 000001b4 0001b4 000013 00 A 0 0 1
> >> [ 2] .note.android.ident NOTE 000001c8 0001c8 000018 00 A 0 0 4
> >> @@ -11,8 +11,8 @@
> >> [ 6] .gnu.version_r VERNEED 0000026c 00026c 000020 00 A 8 1 4
> >> [ 7] .gnu.hash GNU_HASH 0000028c 00028c 000018 00 A 4 0 4
> >> [ 8] .dynstr STRTAB 000002a4 0002a4 000064 00 A 0 0 1
> >> - [ 9] .rel.dyn ANDROID_REL 00000308 000308 00000d 01 A 4 0 4
> >> - [10] .relr.dyn RELR 00000318 000318 00000c 04 A 0 0 4
> >> + [ 9] .rel.dyn LOOS+0x1 00000308 000308 00000d 01 A 4 0 4
> >> + [10] .relr.dyn 00000013: <unknown> 00000318 000318 00000c 04 A 0
> >
> > (yeah, that's probably just a binutils too old to recognize RELR.)
>
> Matching new ones instead of old ones is good, but it has a history of changing
> enough that new binutils versions are likely to break this exact match _again_
> in future, on a similar timescale.
>
> >> 0 4
> >> [11] .ARM.exidx ARM_EXIDX 00000324 000324 000028 00 AL 14 0 4
> >> [12] .rel.plt REL 0000034c 00034c 000020 08 AI 4 19 4
> >> [13] .rodata PROGBITS 0000036c 00036c 000015 01 AMS 0 0 1
> >> @@ -24,7 +24,7 @@
> >> [19] .got.plt PROGBITS 000026a0 0006a0 00001c 00 WA 0 0 4
> >> [20] .data PROGBITS 000036bc 0006bc 00000c 00 WA 0 0 4
> >> [21] .comment PROGBITS 00000000 0006c8 0000cc 01 MS 0 0 1
> >> - [22] .ARM.attributes ATTRIBUTES 00000000 000794 000042 00 0 0 1
> >> + [22] .ARM.attributes ARM_ATTRIBUTES 00000000 000794 000042 00 0 0 1
> >
> > this one's my fault from a recent change --- i left it alone because
> > _really_ when you're decoding stuff in that space you need to _also_
> > check the architecture, and we don't have anything like that atm, and
> > it didn't seem worth it yet.
>
> Exact match is hard. And brittle.
>
> >> [23] .debug_frame PROGBITS 00000000 0007d8 00007a 00 C 0 0 4
> >> [24] .symtab SYMTAB 00000000 000854 000220 10 26 27 4
> >> [25] .shstrtab STRTAB 00000000 000a74 00010e 00 0 0 1
> >> make: *** [.singlemake:809: test_readelf] Error 1
> >>
> >>
> >> (I still haven't switched from devuan bridezilla to devuan deadlock.)
> >
> > that might be the difference here, yes.
> I may have had a bit of tabsplosion the past few months. Gotta close a zillion
> open windows if I don't want the reboot to lose state. I was going to do more of
> that this morning, but I'm working on grep instead.
>
> > the other differences you can make go away by just not using an arm32
> > ELF file (or possibly even just "not built by the NDK").
>
> The ELF file was submitted as part of the test, I don't chose it, but I don't
> mind having that be the test file. My objection is basically test granularity:
> we're asking it to recite the declaration of independence exactly word for word
> when we just want to grab "life, liberty, and the purfuit of happineff" and make
> sure the s's look like f's.
>
> https://www.youtube.com/watch?v=iOOQfGWt8Hc#t=2m20s
>
> > (the ARM_ATTRIBUTES one we might just want to change and admit that
> > our decoding of machine-specific SHT_ values is a bit dodgy.)
>
> I don't currently have the domain expertise to evaluate that, and am not
> following the tangent just now. I trust your judgement on what's right to do there.
now you've pushed, i've just run the ToT tests on debian "testing",
and all the tests you had problems with pass (including this one, so i
guess they switched at some point?).
there _is_ still a failure, but it's a real "they did what?!" one:
```
FAIL: readelf -s
echo -ne '' | readelf -s
/tmp/toybox/tests/files/elf/ndk-elf-note-short | sed s/@.*//
--- expected 2023-11-30 14:51:19.769696679 -0800
+++ actual 2023-11-30 14:51:19.773698857 -0800
@@ -3,8 +3,8 @@
Num: Value Size Type Bind Vis Ndx Name
0: 00000000 0 NOTYPE LOCAL DEFAULT UND
1: 00000000 0 FUNC GLOBAL DEFAULT UND __libc_init
- 2: 00000000 0 FUNC GLOBAL DEFAULT UND __stack_chk_fail
- 3: 00000000 0 OBJECT GLOBAL DEFAULT UND __stack_chk_guard
+ 2: 00000000 0 FUNC GLOBAL DEFAULT UND __stack[...]
+ 3: 00000000 0 OBJECT GLOBAL DEFAULT UND __stack[...]
4: 00000000 0 FUNC GLOBAL DEFAULT UND memset
5: 000010d8 12 FUNC GLOBAL DEFAULT 13 __aeabi_memclr
6: 000010d8 12 FUNC GLOBAL DEFAULT 13 __aeabi_memclr4
```
i mean, they had to do _extra work_ to get _worse_ results! there
really is a GNU version of https://www.youtube.com/watch?v=6LHGY33cFiE
waiting to be written...
i'm pretty tempted to toyonly that test with a comment along the lines of
```
# GNU binutils 2.41 deliberately mangles __stack_chk_fail and
# __stack_chk_guard to __stack[...] for both for some reason,
# which is a bug we shouldn't duplicate, so we filter that out.
```
just like we've toyonly-ed a few of the other tests where binutils
actively _lies_ about what's in the file, which is really the last
thing i want my elf dumping tool to do.
alternatively, adding `| grep -v __stack` before the existing sed (and
deleting the two lines from the expected output) lets us keep running
this on the host too, for what that's worth.
> I can adjust the test myself, lemme get grep checked in first. And I think I
> finally figured out how to redo the shell backslash newline processing on the
> plane and need to finish that. And I need to close all the various open terminal
> tabs to the various todo.txt files and close them all so I can reboot the laptop
> (and swap in the 16 gig ram sticks and reinstall devuan, might want to get a new
> ssd while I'm at it because it's been a few years and they do wear out). And
> finish adding the or1k target (and, sigh, maybe riscv64) to mcm/buildall.sh and
> mkroot so I can build a root filesystem for my orange pi that I'm willing to
> expose to the internet so that can do nightly builds and automated regression
> testing. And I really need to cut the now month-overdue toybox release...
>
> There's already a "make all TEST_HOST pass" todo item, this can get revisited
> there...
>
> Rob
More information about the Toybox
mailing list