Re: Unaligned accesses in sha1dc

["Martin Ågren" <martin.agren@gmail.com>]

On 1 June 2017 at 12:33, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> On Thu, Jun 1, 2017 at 12:26 PM, Martin Ågren <martin.agren@gmail.com> wrote:
>> On 1 June 2017 at 12:08, Andreas Schwab <schwab@suse.de> wrote:
>>> Even if the architecture implements unaligned accesses in hardware, it
>>> is still undefined behaviour, and the compiler will (eventually) take
>>> advantage of it.
>>
>> I tried to optically follow the macros and ended up on line 87/89 in
>> lib/sha1.c of the sha1dc-library, where there is undefined behavior if
>> the address is unaligned, which it seems it could be. Maybe Git uses
>> some particular combination of macro-definitions and I went down the
>> wrong path... There might also be other spots; I haven't thrown UBSan
>> at the code.
>>
>> Using memcpy on those lines should not be a performance problem on
>> platforms where unaligned access is ok, of course assuming the
>> compiler sees the opportunity.
>
> This is what the upstream version of sha1dc now in the next branch
> does, i.e. just does a memcpy() on platforms which aren't on a
> whitelist of CPUs that allow unaligned access.

Ok, now I get it. Undefined behavior can occur on line 1772 in
sha1dc/sha1.c on "next", but only if SHA1DC_ALLOW_UNALIGNED_ACCESS is
defined. I don't think the macro does what its name suggests, though.
To me, it behaves more like
SHA1DC_RELY_ON_UNDEFINED_BEHAVIOR_TO_DO_THE_RIGHT_THING_BY_CHANCE...

So it seems the call chain of commands and macros could be redesigned
to work with "char*" instead of "uint32_t*"... Then the lines I
mentioned earlier could be converted to memcpy and "should" be
compiled to efficient loads where possible. Yes, I know, patches speak
for themselves, and no, this mail is not a patch.