Page MenuHomeFreeBSD

Fix sys.kern.coredump_phnum_test.coredump_phnum on i386
AbandonedPublic

Authored by lwhsu on Dec 10 2018, 6:04 AM.

Details

Summary

The test code wanted to find "0000000000000001 .* 66[0-9]{3} " in readelf -S
output, but it is too long on 32-bit architectures. Change to grab the wanted
section header information and do the actually intended comparsion.


Further information:

Failure information: https://ci.freebsd.org/job/FreeBSD-head-i386-test/3837/testReport/sys.kern/coredump_phnum_test/coredump_phnum/

Output on amd64:

root@:/usr/tests/sys/kern # readelf -S coredump_phnum_helper.core | grep -A1 "^  \[ 0\] "
  [ 0] <no-name>         NULL             0000000000000000  00000000
       0000000000000001  0000000000000000           0   66546     0

Output on i386:

root@:/usr/tests/sys/kern # readelf -S coredump_phnum_helper.core | grep -A1 "^  \[ 0\] "
  [ 0] <no-name>         NULL            00000000 000000 000001 00      0 66545  0
Key to Flags:

We can also use:

atf_check -x "test `readelf -SW coredump_phnum_helper.core awk '{print $10}'` -gt 65535"

or

atf_check -o "match: 000001 .* 66[0-9]{3} " \
	-x 'readelf -SW coredump_phnum_helper.core | grep "^  \[ 0\] "'

but I feel the proposed one is easier to read. I'm also fine to chenge to other
approach to follow the style.

Test Plan

cd /usr/tests/sys/kern && kyua debug coredump_phnum_test:coredump_phnum

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 21468
Build 20786: arc lint + arc unit

Event Timeline

lwhsu created this revision.Dec 10 2018, 6:04 AM
cem resigned from this revision.Dec 10 2018, 6:58 PM

I'm not familiar with this tool or the test anymore, and not interested in studying it again now. I'm sure your fix is fine, but I am not a good reviewer.

markj added inline comments.Dec 10 2018, 7:12 PM
tests/sys/kern/coredump_phnum_test.sh
68

This is a bit fragile since you're assuming that the core file contains exactly one section. We should probably verify this first.

Also, as a minor nit, I would grep for ^[[:space:]]*sh_info:.

ngie requested changes to this revision.Dec 12 2018, 4:58 PM

I remember this test being fragile because it relies on output from a third-party application under a large amount of development and makes assumptions based on a hardcoded value which cannot be determined programmatically. This is something that I tried getting more context in the past, but the ball was dropped with this item. I very much understand how this is broken though, and I will try to think of a better way to check for this (the right solution might involve writing a C program which emulates a stripped down version of readelf, etc).

I don't think we should move forward with this change. In particular, your proposed approach will be hard to diagnose after the fact as well if the number of columns output or other formatting changes: the test will fail with difficult to understand diagnostic messages (in particular if the field is non-numeric); you can't test readelf to make sure it's executing cleanly; etc.

This revision now requires changes to proceed.Dec 12 2018, 4:58 PM

readelf is under our control and is largely unchanging, we should be able to rely on it

ngie added a comment.Dec 16 2018, 6:21 AM

readelf is under our control and is largely unchanging, we should be able to rely on it

elfdump we do as well, but I'm a bit nervous about switching over to elfdump from readelf.

ngie added a comment.Apr 22 2019, 10:15 AM

I've proposed a different solution in D20005.

lwhsu abandoned this revision.Apr 23 2019, 2:58 AM

Suppressed by D20005.