Re: [PATCH v2] block-sha1: remove use of assembly

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Wed, Mar 09 2022, Taylor Blau wrote:

> On Wed, Mar 09, 2022 at 10:10:33PM +0000, brian m. carlson wrote:
>> On 2022-03-08 at 13:38:06, Ævar Arnfjörð Bjarmason wrote:
>> >
>> > On Tue, Mar 08 2022, brian m. carlson wrote:
>> >
>> > I think the $subject of the patch needs updating. It's not removing all
>> > the assemply from the file, after this patch we still have the
>> > ARM-specific assembly.
>> >
>> > I don't have a box to test that on, but I wonder if that also triggers
>> > the pedantic mode?
>> >
>> > Perhaps:
>> >
>> >     block-sha1: remove superfluous i386 and x86-64 assembly
>>
>> I suspect it has the same problem.  My inclination is to just remove it,
>> because my guess is that the compiler has gotten smarter between 2009
>> and now.
>
> Almost certainly. I don't have a machine to test it on, either, but I
> would be shocked if `make BLK_SHA=YesPlease DEVELOPER=1` worked on
> master today on an arm machine.

Why is that? The -pedantic error is specifically about
"gnu-statement-expression", i.e. the bracket syntax, not the inline
assembly per-se.

The ARM assembly isn't using that, and we have other code __asm__ code
compiled with -pedantic. E.g. I get the __asm__ in "compat/bswap.h" by
default, and it passes -pedantic (the code starting around line 38).

>> I honestly intend to just remove this code in a future version because
>> everyone not using SHA1DC has a security problem and we shouldn't offer
>> insecure options.
>>
>> However, I think for now, I'm just going to reroll this with the new
>> title and then I can remove it in a future version unless somebody with
>> an ARM system can relatively quickly tell me whether it's necessary.
>
> I wonder if a good stop-gap for arm systems might be to do something
> like:
>
> --- 8< ---
>
> diff --git a/block-sha1/sha1.c b/block-sha1/sha1.c
> index 1bb6e7c069..7402d02875 100644
> --- a/block-sha1/sha1.c
> +++ b/block-sha1/sha1.c
> @@ -57,7 +57,7 @@
>  #if defined(__i386__) || defined(__x86_64__)
>    #define setW(x, val) (*(volatile unsigned int *)&W(x) = (val))
>  #elif defined(__GNUC__) && defined(__arm__)
> -  #define setW(x, val) do { W(x) = (val); __asm__("":::"memory"); } while (0)
> +  #define setW(x, val) do { W(x) = (val); __extension__ __asm__("":::"memory"); } while (0)
>  #else
>    #define setW(x, val) (W(x) = (val))
>  #endif
>
> --- >8 ---

Isn't that __extension__ only needed *if* it warns under -pedantic,
which AFAICT doesn't apply to all uses of __asm__ (but your compiler
version etc. may be different...).