On 1 June 2017 at 12:33, Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> wrote: > On Thu, Jun 1, 2017 at 12:26 PM, Martin Ågren <martin.agren@xxxxxxxxx> wrote: >> On 1 June 2017 at 12:08, Andreas Schwab <schwab@xxxxxxx> 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.