[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Sat, 18 Apr 2015 01:30:04 +0200
From: magnum <john.magnum@...hmail.com>
To: john-dev@...ts.openwall.com
Subject: Re: Format interface changes
On 2015-04-17 02:05, Alexander Cherepanov wrote:
> On 2015-04-16 22:08, magnum wrote:
>> I think it may be about time we change key index/count variables to
>> unsigned int in Jumbo. Rationale:
>> (...)
> I generally agree that we should use unsigned more. I think it makes
> many things easier. It's great that you even have hard data on speed
> gain. Maybe change "char *ciphertext" to "unsigned char *ciphertext"?
Yes, nearly all uses of char in JtR should be unsigned. It has
irritating side effects though - like having to cast any strlen() or
you'd get compiler warnings. BTW I hate that - when would that EVER be a
valuable warning? IMO the few systems that has char default to unsigned
has it right (as long as it's called "char" anyway) but there isn't much
point in having any opinion about it :-)
> Another couple of wild ideas:
>
> - add restrict to help compiler optimize code where we are sure that
> parameters cannot overlap. Many parameters are of type char * and I
> guess compiler cannot reason about its aliasing because char can be used
> to access any data;
WE can't reason about it either! Not in the interface. It's a contract
with the devil. Besides, I see no place in the format interface where it
would do any good.
We can still use it carefully without any change to the interface but
it's more dangerous than you'd think - consider this patch to
nt2_fmt_plug.c:
diff --git a/src/nt2_fmt_plug.c b/src/nt2_fmt_plug.c
--- a/src/nt2_fmt_plug.c
+++ b/src/nt2_fmt_plug.c
@@ -286,8 +286,8 @@
static void set_key(char *_key, int index)
{
#ifdef SIMD_COEF_32
- const unsigned char *key = (unsigned char*)_key;
- unsigned int *keybuf_word = buf_ptr[index];
+ const unsigned char *__restrict key = (unsigned char*)_key;
+ unsigned int *__restrict keybuf_word = buf_ptr[index];
unsigned int len, temp2;
len = 0;
(...)
((unsigned int *)saved_key)[14*SIMD_COEF_32 + (index&(SIMD_COEF_32-1))
+ index/SIMD_COEF_32*16*SIMD_COEF_32] = len << 4;
It looks clever but does absolutely nothing for performance. However,
we're actually breaking the contract because saved_key is aliased by
buf_ptr. The bug may surface years later on some obscure compiler.
Admittedly in this case I can't see how it would ever surface since we
depend on "len", but this example shows how easy it is to miss. And
anyway we ARE still breaking the contract.
We could try using it for a few helper functions though, like strzcpy()
and strlen8(). But it wont cause any change to the format struct so
that's a separate project (we should only commit such changes once we
see significant gain and/or are 101% sure we're not mistaken about
aliasing - which means reviewing all callers and then review them again).
> - is there need for parameters to be aligned? If many formats will
> benefit from this we can enforce it at the level of the interface.
We already do in salt/binary functions. While set_key() would benefit in
some cases (actually very few - perhaps only raw-sha1-ng and only on
older CPUs), it would slow down some modes more than it would boost
others. Newish x86 can do unaligned load (even SIMD) with no performance
hit. On a side note, MIC can't do it at all - Lei had to emulate that
intrinsic. But I think that is a paren, I assume AVX512 will have it.
magnum
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
