Bug 215422 - bhnd.ko does not build reproducibly
Summary: bhnd.ko does not build reproducibly
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: freebsd-bugs mailing list
URL: https://reviews.freebsd.org/D8859
Keywords: patch
Depends on:
Blocks:
 
Reported: 2016-12-19 17:56 UTC by Ed Maste
Modified: 2016-12-21 15:47 UTC (History)
0 users

See Also:


Attachments
diffoscope output comparing two bhnd builds (673.47 KB, text/plain)
2016-12-19 17:56 UTC, Ed Maste
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ed Maste freebsd_committer 2016-12-19 17:56:00 UTC
Created attachment 178108 [details]
diffoscope output comparing two bhnd builds

Successive iterations of 'make buildkernel' produced differing bhnd.ko & bhnd.ko.debug, as in the attached diffoscope output. Additional details to be added after further investigation.
Comment 1 Ed Maste freebsd_committer 2016-12-19 18:48:25 UTC
It seems nvram_map_gen.sh -d has non-deterministic output.

I ran "sh sys/dev/bhnd/tools/nvram_map_gen.sh sys/dev/bhnd/nvram/nvram_map -d" fifty times, and produced eight different versions of bhnd_nvram_map_data.h.

For example:

% diff -u3 bhnd_nvram_map_data.h.10 bhnd_nvram_map_data.h.28
--- bhnd_nvram_map_data.h.10    2016-12-19 13:43:52.861866000 -0500
+++ bhnd_nvram_map_data.h.28    2016-12-19 13:45:04.762683000 -0500
@@ -620,7 +620,7 @@
        /* ag1 (0x75) */
        (SPROM_OPCODE_VAR_IMM|BHND_NVAR_AG1_ID),
        (SPROM_OPCODE_DO_BIND|0|(1<<SPROM_OP_BIND_SKIP_IN_SHIFT)|(1<<SPROM_OP_BIND_SKIP_OUT_SHIFT)),
-       (SPROM_OPCODE_REV_IMM|2),
+       (SPROM_OPCODE_REV_IMM|3),
        /* bind (u8 @ 0x75 -> 0x76) */
        /* boardflags (0x76) */
        (SPROM_OPCODE_VAR_IMM|BHND_NVAR_BOARDFLAGS_ID),
@@ -629,31 +629,30 @@
                114,
                (SPROM_OPCODE_DO_BIND|0|(1<<SPROM_OP_BIND_SKIP_IN_SHIFT)|(0<<SPROM_OP_BIND_SKIP_OUT_SHIFT)),
                /* bind (u16 @ 0x72 -> 0x74) */
-               (SPROM_OPCODE_OFFSET|SPROM_OP_DATA_U8),
-               56,
+               /* 0x74 + 0x6 -> 0x7a */
+               (SPROM_OPCODE_OFFSET_REL_IMM|3),
                (SPROM_OPCODE_SHIFT|SPROM_OP_DATA_I8),
                -16,
        (SPROM_OPCODE_DO_BIND|0|(1<<SPROM_OP_BIND_SKIP_IN_SHIFT)|(1<<SPROM_OP_BIND_SKIP_OUT_SHIFT)),
-       (SPROM_OPCODE_REV_IMM|3),
-       /* bind (u16 @ 0x38 -> 0x3a) */
-       /* boardflags (0x3a) */
+       (SPROM_OPCODE_REV_IMM|2),
+       /* bind (u16 @ 0x7a -> 0x7c) */
+       /* boardflags (0x7c) */
        (SPROM_OPCODE_VAR_IMM|BHND_NVAR_BOARDFLAGS_ID),
                SPROM_OPCODE_TYPE_IMM|BHND_NVRAM_TYPE_UINT16,
                (SPROM_OPCODE_OFFSET|SPROM_OP_DATA_U8),
                114,
                (SPROM_OPCODE_DO_BIND|0|(1<<SPROM_OP_BIND_SKIP_IN_SHIFT)|(0<<SPROM_OP_BIND_SKIP_OUT_SHIFT)),
                /* bind (u16 @ 0x72 -> 0x74) */
-               /* 0x74 + 0x6 -> 0x7a */
-               (SPROM_OPCODE_OFFSET_REL_IMM|3),
+               (SPROM_OPCODE_OFFSET|SPROM_OP_DATA_U8),
+               56,
                (SPROM_OPCODE_SHIFT|SPROM_OP_DATA_I8),
                -16,
        (SPROM_OPCODE_DO_BIND|0|(1<<SPROM_OP_BIND_SKIP_IN_SHIFT)|(1<<SPROM_OP_BIND_SKIP_OUT_SHIFT)),
-       (SPROM_OPCODE_REV_IMM|2),
-       /* bind (u16 @ 0x7a -> 0x7c) */
-       /* boardnum (0x7c) */
+       /* bind (u16 @ 0x38 -> 0x3a) */
+       /* boardnum (0x3a) */
        (SPROM_OPCODE_VAR_REL_IMM|3),
-               (SPROM_OPCODE_OFFSET|SPROM_OP_DATA_U8),
-               76,
+               /* 0x3a + 0x12 -> 0x4c */
+               (SPROM_OPCODE_OFFSET_REL_IMM|9),
        (SPROM_OPCODE_DO_BIND|0|(1<<SPROM_OP_BIND_SKIP_IN_SHIFT)|(1<<SPROM_OP_BIND_SKIP_OUT_SHIFT)),
        (SPROM_OPCODE_REV_IMM|3),
        /* bind (u16 @ 0x4c -> 0x4e) */
Comment 2 Ed Maste freebsd_committer 2016-12-19 19:24:40 UTC
lidl@ noted the use of srand(), used to select the qsort pivot. The output is reproducible for me with the following diff:
 
diff --git a/sys/dev/bhnd/tools/nvram_map_gen.awk b/sys/dev/bhnd/tools/nvram_map_gen.awk
index 834bd1f..74fa58f 100755
--- a/sys/dev/bhnd/tools/nvram_map_gen.awk
+++ b/sys/dev/bhnd/tools/nvram_map_gen.awk
@@ -52,9 +52,6 @@ function main(_i) {
                AWK_REQ_HEX_PARSING=1
        }
 
-       # Seed rand()
-       srand()
-
        # Output type
        OUT_T = null
        OUT_T_HEADER = "HEADER"
Comment 3 Ed Maste freebsd_committer 2016-12-19 19:30:58 UTC
> lidl@ noted the use of srand(), used to select the qsort pivot.

Rather, rand() is used to select the qsort pivot. The script initializes awk's random number generator using the current time, via srand().
Comment 4 Ed Maste freebsd_committer 2016-12-20 02:59:12 UTC
IMO landonf has the right fix in https://reviews.freebsd.org/D8859, although it seems reasonable to apply https://reviews.freebsd.org/D8857 as well.
Comment 5 commit-hook freebsd_committer 2016-12-21 00:51:14 UTC
A commit references this bug:

Author: landonf
Date: Wed Dec 21 00:50:21 UTC 2016
New revision: 310342
URL: https://svnweb.freebsd.org/changeset/base/310342

Log:
  bhnd(4): Use a stable sort key to produce deterministic nvram_map_gen.awk
  output.

  When ordering SROM layout entries, we now use the unique (var_id,
  rev_start, rev_end) tuple as the sort key; this fixes the previously
  non-deterministic output when sorting entries with overlapping var_ids.

  PR:		215422
  Reported by:	emaste
  Reviewed by:	emaste
  Approved by:	adrian (mentor)
  Differential Revision:	https://reviews.freebsd.org/D8859

Changes:
  head/sys/dev/bhnd/tools/nvram_map_gen.awk
Comment 6 commit-hook freebsd_committer 2016-12-21 15:46:18 UTC
A commit references this bug:

Author: emaste
Date: Wed Dec 21 15:45:23 UTC 2016
New revision: 310371
URL: https://svnweb.freebsd.org/changeset/base/310371

Log:
  bhnd: remove srand() to ensure deterministic output

  r310342 fixed non-deterministic nvram_map_gen.awk output and thus a non-
  reproducible bhnd(4) build by using a unique sort key.

  Go one step further and also remove the srand() call. There's no reason
  we want non-deterministic behaviour from this script.

  PR:		215422
  Reported by:	gjb (non-reproducibility of bhnd)
  Reported by:	lidl (srand as the cause)
  Reviewed by:	landonf
  Sponsored by:	The FreeBSD Foundation
  Differential Revision:	https://reviews.freebsd.org/D8857

Changes:
  head/sys/dev/bhnd/tools/nvram_map_gen.awk