Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

One fixup and one commit message change -- to cherry-pick, not to merge #1

Closed
wants to merge 3 commits into from

Conversation

j6t
Copy link

@j6t j6t commented Aug 10, 2017

Please consider the two top-most commits here for inclusion. Please cherry-pick the fixup! commit and consider to apply the commit message of the reword! commit to the respective commit mentioned in the subject lines.

j6t added 3 commits August 10, 2017 09:41
The MSYS2 less is GNU's version. It is so much more capable than the
BusyBox version. Do not override it.

Signed-off-by: Johannes Sixt <[email protected]>
Macros guarding conditional bash compatibility code are always defined.
Checking whether a macro is defined is, therefore, useless. Check the
expansion value.

Signed-off-by: Johannes Sixt <[email protected]>
new Subject:
libarchive: avoid type punning corner case

When compiling xz_dec_stream.c with GCC (at least with versions 5.4.0
and 7.1.0), it complains thusly:

        In function 'dec_stream_footer':
        error: dereferencing type-punned pointer will break
              strict-aliasing rules [-Werror=strict-aliasing]
           if (xz_crc32(s->temp.buf + 4, 6, 0) != get_le32(s->temp.buf))
               ^~

Despite the arrow pointing to the xz_crc32() call, the actual problem is
the get_le32() macro expansion.

The `buf` field is an array of uint8_t. In this case, the macro expansion
amounts to a cast of the array to a pointer to an unrelated type. It looks
like GCC does not decay the array to a pointer before it applies the cast.

We could just write the sub-expression as get_le32(s->temp.buf + 0), which
forces the array to decay to a pointer. But future readers would have to
be enlightened about the purpose the + 0 with a comment.

Let's do it differently: Turn the get_le32() macro into an inline
function taking a pointer parameter. Then the array-to-pointer decay
happens as part of the argment passing and without funny gymnastics.

Signed-off-by: Johannes Schindelin <[email protected]>
Signed-off-by: Johannes Sixt <[email protected]>
@@ -10260,7 +10260,7 @@ evalcommand(union node *cmd, int flags)

expredir(cmd->ncmd.redirect);
redir_stop = pushredir(cmd->ncmd.redirect);
#ifdef BASH_XTRACEFD
#if BASH_XTRACEFD

This comment was marked as off-topic.

@dscho
Copy link
Member

dscho commented Aug 10, 2017

Seems that there is no place to comment on a commit message. So here goes:

We could just write the sub-expression as get_le32(s->temp.buf + 0)

No, this is incorrect. I tested this specifically, and it left the compiler warning unchanged.

It was discussed on the BusyBox mailing list starting with my original patch submission and it was even determined that there is a bug fix for GCC relating to this.

However, it was also determined that an easier fix (at the possible expense of a slight performance hit) is to use get_unaligned_le32() instead, dropping my patch altogether.

That leaves only the patch that disables the less applet. Would you kindly rebase, keeping only that patch? I'll gladly merge after that.

Thanks!

@j6t
Copy link
Author

j6t commented Aug 10, 2017

I am sorry, I'm still getting warm with a workflow. I forgot to cross-check my claim that get_le32(s->temp.buf + 0) would not produce a warning. My main concern about the commit message is, however, that you were talking about alignment, but GCC's warning has nothing to do with alignment; it is all about the cast. Maybe you can reword that?

I do not want to submit the third patch that disables the less applet, yet, because I'm not yet convinced that it is a good move.

I'll close this pull request then.

@j6t j6t closed this Aug 10, 2017
@dscho
Copy link
Member

dscho commented Aug 10, 2017

I am sorry, I'm still getting warm with a workflow.

No worries. It is a complex setup, with two upstreams, one of which makes it easy to contribute.

My main concern about the commit message is, however, that you were talking about alignment, but GCC's warning has nothing to do with alignment; it is all about the cast. Maybe you can reword that?

As I stated earlier, the BusyBox team decided to do a much smaller thing and drop the unaliased get_le32() completely. When that fix comes into this here repository, there will not be any commit left whose commit message I could reword 😄

dscho pushed a commit that referenced this pull request Sep 19, 2022
This was missing in the previous attempt to fix it via [1]

This helps fix segfaults when compiling with clang ( seen on riscv64 )

[  452.428349] less[270]: unhandled signal 11 code 0x1 at 0x000000000000000c in busybox.nosuid[2ab7491000+ba000]
[  452.430246] CPU: 3 PID: 270 Comm: less Not tainted 5.15.13-yocto-standard #1
[  452.431323] Hardware name: riscv-virtio,qemu (DT)
[  452.431925] epc : 0000002ab74a19ee ra : 0000002ab74a19dc sp : 0000003fec6ec980
[  452.432725]  gp : 0000002ab754dcb0 tp : 0000003f88783800 t0 : 0000003f8878d4a0
[  452.433744]  t1 : 0000002ab749b00c t2 : 0000000000000000 s0 : 0000003fec6ecc38
[  452.434732]  s1 : 000000000000004c a0 : 00000000ffffffff a1 : 0000002ab754dde0
[  452.435861]  a2 : 0000000000000000 a3 : 0000000000000100 a4 : 0000002ab754f3a0
[  452.436787]  a5 : 0000002ab754f3a0 a6 : 0000000000000000 a7 : 0000002ab754f2a0
[  452.437974]  s2 : 0000000000000002 s3 : 0000002ab754b6c8 s4 : 0000002ab749b60e
[  452.438781]  s5 : 0000000000000000 s6 : 0000002ab754b6c8 s7 : 0000003f88943060
[  452.439723]  s8 : 0000003f88944050 s9 : 0000002ad8502e88 s10: 0000002ad8502de8
[  452.440538]  s11: 0000000000000014 t3 : 0000003f887fceb6 t4 : 0000003f8893af0c
[  452.441438]  t5 : 0000000000000000 t6 : 0000003f88923000

[1] https://git.busybox.net/busybox/commit/?id=1f925038a

Signed-off-by: Khem Raj <[email protected]>
Signed-off-by: Denys Vlasenko <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants