ar: shuffle symbol offsets during conversion for 32-bit ar archives

[emaste]

Summary

This is a merge of three commits from my working tree:

commit 363f3194b2f0f229bdd8a2295319252acfb033f8
Author: Ed Maste <[e-mail]>
Date:   Mon Apr 22 09:39:26 2019 -0400

    ar: test for writing 64-bit format only if symbol count is nonzero

    This is a minor simplification; if we do not have any symbols a 32-bit
    (empty) symbol table is sufficient.

    Sponsored by:   The FreeBSD Foundation

commit 01214a87fa0b029b26d19bb018c43e2f6c480b9e
Author: Ed Maste <[e-mail]>
Date:   Mon Apr 22 09:44:12 2019 -0400

    ar: use array notation to access s_so

    This is somewhat more readable than pointer arithmetic.

    Sponsored by:   The FreeBSD Foundation

commit 17c24416b6bd478c947918c8cc94f19255c651e2 (HEAD -> wipbsd.20190326)
Author: Ed Maste <[e-mail]>
Date:   Mon Apr 22 09:53:12 2019 -0400

    ar: shuffle symbol offsets during conversion for 32-bit ar archives

    During processing we maintain symbol offsets in the 64-bit s_so array,
    and when writing the archive convert to 32-bit if no offsets are greater
    than 4GB.  However, this was inefficient as we looped over the array
    twice: first, converting to big endian and second, writing each 32-bit
    value one at a time.  Instead, when writing a 32-bit archive shuffle
    convert symbol data to big endian and shuffle to the beginning of the
    array at the same time.

    Sponsored by:   The FreeBSD Foundation

[emaste]

usr.bin/ar/write.c

679 ↗

(On Diff #56484)

XXX

[emaste]

oh still an endian bug though,

if (w_sz == sizeof(uint32_t))
        nr = (uint64_t)htobe32((uint32_t)bsdar->s_cnt);
else
        nr = htobe64(bsdar->s_cnt);
write_data(bsdar, a, &nr, w_sz);

[emaste]

Rework writing s_cnt to avoid 32-bit big endian issues

In the 32-bit case previous code did:

unit64_t nr;
nr = (uint64_t)htobe32((uint32_t)bsdar->s_cnt);
write_data(bsdar, a, &nr, sizeof(uint32_t));

which on BE platforms wrote the upper four zero bytes of nr

[aryeeteygerald_rogers.com]

I like the changes.

[emaste]

Rebase after committing ar: test for writing 64-bit format only if symbol count is nonzero and ar: use array notation to access s_so. Fix build, add missing & in &nr32.

[emaste]

Add UB comment in source

[emaste]

usr.bin/ar/write.c

683–684 ↗

(On Diff #56501)

For reference clang generates this nice asm:

     e10:       8b 3c f2        movl    (%rdx,%rsi,8), %edi
; htobe32(bsdar->s_so[i] + pm_sz);
     e13:       01 cf   addl    %ecx, %edi
     e15:       0f cf   bswapl  %edi
; ((uint32_t *)(bsdar->s_so))[i] =
     e17:       89 3c b2        movl    %edi, (%rdx,%rsi,4)
; for (i = 0; (size_t)i < bsdar->s_cnt; i++)
     e1a:       48 83 c6 01     addq    $1, %rsi
     e1e:       48 39 f0        cmpq    %rsi, %rax
     e21:       75 ed   jne     -19 <write_archive+0xe00>

[cem]

usr.bin/ar/write.c

682 ↗

(On Diff #56501)

Can i be size_t? It appears to be int, which is why this cast is present, but size_t is definitely more correct.

683–684 ↗

(On Diff #56501)

As discussed on IRC this is an aliasing violation, but the compiler doesn't seem to take advantage of that and generate surprising code, for now.

Possible non-UB solutions would be using be32enc() (not portable, but easily embedded); (buffered) directly writing out the big endian values; or allocating a 2nd array for the 32-bit compaction result.

[jhibbits]

Works for me, thanks!

[emaste]

usr.bin/ar/write.c

682 ↗

(On Diff #56501)

Changed in my WIP tree.

683–684 ↗

(On Diff #56501)

Yes, I think the best way to do it is malloc a new buffer in write_objs of the appropriate size and do not do the in-place modification for either 32- or 64-bit output.