Page MenuHomeFreeBSD
Differential D20007

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

Authored by emaste on Apr 22 2019, 2:08 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Apr 6, 12:08 PM
Unknown Object (File)
Sat, Apr 6, 11:56 AM
Unknown Object (File)
Sat, Apr 6, 11:14 AM
Unknown Object (File)
Sat, Apr 6, 8:23 AM
Unknown Object (File)
Sat, Apr 6, 7:25 AM
Unknown Object (File)
Sat, Apr 6, 7:11 AM
Unknown Object (File)
Feb 13 2024, 2:16 AM
Unknown Object (File)
Dec 28 2023, 12:42 PM
View All 42 Files
Subscribers
aryeeteygerald_rogers.com
cem
imp

Details

Reviewers
jhibbits
linimon
aryeeteygerald_rogers.com
Commits
rS346582: ar: shuffle symbol offsets during conversion for 32-bit ar archives
Summary

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

commit 363f3194b2f0f229bdd8a2295319252acfb033f8
Author: Ed Maste <emaste@freebsd.org>
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 <emaste@freebsd.org>
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 <emaste@freebsd.org>
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

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

emaste created this revision.Apr 22 2019, 2:08 PM
emaste added inline comments.Apr 22 2019, 2:12 PM
usr.bin/ar/write.c
682

XXX

emaste added a comment.Apr 22 2019, 2:19 PM

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 updated this revision to Diff 56485.Apr 22 2019, 2:33 PM
emaste added a subscriber: aryeeteygerald_rogers.com.

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 accepted this revision.Apr 22 2019, 4:23 PM

I like the changes.

This revision is now accepted and ready to land.Apr 22 2019, 4:23 PM
emaste updated this revision to Diff 56498.Apr 22 2019, 5:35 PM

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.

This revision now requires review to proceed.Apr 22 2019, 5:35 PM
emaste updated this revision to Diff 56501.Apr 22 2019, 6:11 PM

Add UB comment in source

emaste added inline comments.Apr 22 2019, 6:24 PM
usr.bin/ar/write.c
683–684

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 added a subscriber: cem.Apr 22 2019, 7:22 PM
cem added inline comments.
usr.bin/ar/write.c
682

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

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 accepted this revision.Apr 22 2019, 7:39 PM

Works for me, thanks!

This revision is now accepted and ready to land.Apr 22 2019, 7:39 PM
Closed by commit rS346582: ar: shuffle symbol offsets during conversion for 32-bit ar archives (authored by emaste). · Explain WhyApr 22 2019, 7:56 PM
This revision was automatically updated to reflect the committed changes.
Herald added a subscriber: imp. · View Herald TranscriptApr 22 2019, 7:56 PM
emaste added inline comments.Apr 22 2019, 8:28 PM
usr.bin/ar/write.c
682

Changed in my WIP tree.

683–684

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.

Revision Contents
Changeset List

PathSize
usr.bin/
ar/
37 lines

Diff 56501

View Options

usr.bin/ar/write.c

Loading...