[389-devel] Re: csiphash on Sparc
On Thu, 2017-10-26 at 12:47 +0200, Lukas Slebodnik wrote:
> On (13/10/17 20:35), William Brown wrote:
> > On Wed, 2017-10-11 at 13:36 +0200, Lukas Slebodnik wrote:
> > > But following code should work. Please correct me if I am wrong.
> > > I didn't test.
> > > char *str = strdup("ABCDEFGH12345678");
> > > char *key = malloc(16);
> > >
> > > yes, function sds_siphash13 is not ideal because it rely on
> > > properly alligned
> > > input data.
> > >
> >
> > We are free to change the signature of the function, it's just that
> > I
> > used this from another open source component (thus why it's
> > slightly
> > different style wise)
> >
>
> Changing signature will not help. The main problem is that
> implementation of
> function is not really portable
>
> > sds_siphash13(const void *src, size_t src_sz, const char key[16])
> > {
> > const uint64_t *_key = (uint64_t *)key;
>
> ^^^
> This cast is portable only in case of "((intptr_t)key) %
> sizeof(uint64_t) == 0"
> It is not a problem in intel/amd but it is not portable
> > uint64_t k0 = _le64toh(_key[0]);
> > uint64_t k1 = _le64toh(_key[1]);
> > uint64_t b = (uint64_t)src_sz << 56;
> > const uint64_t *in = (uint64_t *)src;
>
> ^^^
> and the same situation is here
>
> This is usually not the problem is you directly pass pointer returned
> by
> malloc. But it is not the case on many places.
>
> e.g.
> here is an usage on function in libsds
>
> sh$ git grep sds_siphash13 src/libsds/
> src/libsds/external/csiphash/csiphash.c:sds_siphash13(const void
> *src, size_t src_sz, const char key[16])
> src/libsds/include/sds.h: * sds_siphash13 provides an implementation
> of the siphash algorithm for use in
> src/libsds/include/sds.h:uint64_t sds_siphash13(const void *src,
> size_t src_sz, const char key[16]);
> src/libsds/sds/ht/op.c:uint64_t hashout = sds_siphash13(key,
> ht_ptr->key_size_fn(key), ht_ptr->hkey);
> src/libsds/sds/ht/op.c:uint64_t ex_hashout =
> sds_siphash13(slot->slot.value->key, ht_ptr->key_size_fn(slot-
> >slot.value->key), ht_ptr->hkey);
> src/libsds/sds/ht/op.c:uint64_t hashout = sds_siphash13(key,
> ht_ptr->key_size_fn(key), ht_ptr->hkey);
> src/libsds/sds/ht/op.c:uint64_t hashout = sds_siphash13(key,
> ht_ptr->key_size_fn(key), ht_ptr->hkey);
> src/libsds/test/test_sds_csiphash.c:hashout =
> sds_siphash13(&value, sizeof(uint64_t), key);
> src/libsds/test/test_sds_csiphash.c:hashout = sds_siphash13(test,
> 4, key);
>
> ht_ptr->hkey is used on many places and it is not aligned to 64
> bites.
>
> Here is a definition of structure
> typedef struct _sds_ht_instance
> {
> uint32_t checksum;
> char hkey[16];
> sds_ht_node *root;
> int64_t (*key_cmp_fn)(void *a, void *b);
> uint64_t (*key_size_fn)(void *key);
> void *(*key_dup_fn)(void *key);
> void (*key_free_fn)(void *key);
> void *(*value_dup_fn)(void *value);
> void (*value_free_fn)(void *value);
> } sds_ht_instance;
>
> This structure is always created with malloc. So the beginning of
> structure is
> aligned. "ht_ptr = sds_calloc(sizeof(sds_ht_instance));"
> But it is very possible that hkey will not be aligned to 64 bits
> unless
> compiler adds some padding between "checksum" and "hkey".
> You might do the trick to change order of members in structure which
> would
> solve problem here. But it still would not solve problem with
> improperly
> aligned input in sds_siphash13.
>
> let say we have following code
> int main(void)
> {
> char *input = strdup("lorem impsum");
> char *key = calloc(1, 16);
>
> uint64_t temp;
>
> /* this version would not crash) */
> temp = sds_siphash13(input, strlen(input), key);
>
> /* but this version will crash */
> temp = sds_siphash13(input + 1, strlen(input +1), key);
>
> return 0;
> }
>
> It is really difficult to rely on proper alignment of input.
> In theory you could have assert in code which would catch it
> also in x86_64 architecture. But it is not nice solution.
>
> The only portable way is to use memcpy to copy data from
> array of char to variable/array of uint64_t.
>
> BTW recently I read nice article and it can crash even on x86_64
> in case of vector operations. Which compilers try to use as part of
> optimisations.
> https://pzemtsov.github.io/2016/11/06/bug-story-alignment-on-x86.html
>
> And as you can see in article usage of memcpy is not so bad
> especially
> if compiler uses builtin instead of procedure call.
>
> HTH and sorry for late reply.
>
>
Thanks for the great and thorough answer mate. I'm not feeling so good
this week, but I'll read this through and write up a patch early next
week for this. Thanks again!
--
Sincerely,
William Brown
Software Engineer
Red Hat, Australia/Brisbane
signature.asc
Description: This is a digitally signed message part
__
[389-devel] Re: csiphash on Sparc
On (13/10/17 20:35), William Brown wrote:
>On Wed, 2017-10-11 at 13:36 +0200, Lukas Slebodnik wrote:
>> But following code should work. Please correct me if I am wrong. I didn't
>> test.
>> char *str = strdup("ABCDEFGH12345678");
>> char *key = malloc(16);
>>
>> yes, function sds_siphash13 is not ideal because it rely on properly alligned
>> input data.
>>
>
>We are free to change the signature of the function, it's just that I
>used this from another open source component (thus why it's slightly
>different style wise)
>
Changing signature will not help. The main problem is that implementation of
function is not really portable
>sds_siphash13(const void *src, size_t src_sz, const char key[16])
>{
>const uint64_t *_key = (uint64_t *)key;
^^^
This cast is portable only in case of "((intptr_t)key) % sizeof(uint64_t)
== 0"
It is not a problem in intel/amd but it is not portable
>uint64_t k0 = _le64toh(_key[0]);
>uint64_t k1 = _le64toh(_key[1]);
>uint64_t b = (uint64_t)src_sz << 56;
>const uint64_t *in = (uint64_t *)src;
^^^
and the same situation is here
This is usually not the problem is you directly pass pointer returned by
malloc. But it is not the case on many places.
e.g.
here is an usage on function in libsds
sh$ git grep sds_siphash13 src/libsds/
src/libsds/external/csiphash/csiphash.c:sds_siphash13(const void *src, size_t
src_sz, const char key[16])
src/libsds/include/sds.h: * sds_siphash13 provides an implementation of the
siphash algorithm for use in
src/libsds/include/sds.h:uint64_t sds_siphash13(const void *src, size_t src_sz,
const char key[16]);
src/libsds/sds/ht/op.c:uint64_t hashout = sds_siphash13(key,
ht_ptr->key_size_fn(key), ht_ptr->hkey);
src/libsds/sds/ht/op.c:uint64_t ex_hashout =
sds_siphash13(slot->slot.value->key,
ht_ptr->key_size_fn(slot->slot.value->key), ht_ptr->hkey);
src/libsds/sds/ht/op.c:uint64_t hashout = sds_siphash13(key,
ht_ptr->key_size_fn(key), ht_ptr->hkey);
src/libsds/sds/ht/op.c:uint64_t hashout = sds_siphash13(key,
ht_ptr->key_size_fn(key), ht_ptr->hkey);
src/libsds/test/test_sds_csiphash.c:hashout = sds_siphash13(&value,
sizeof(uint64_t), key);
src/libsds/test/test_sds_csiphash.c:hashout = sds_siphash13(test, 4, key);
ht_ptr->hkey is used on many places and it is not aligned to 64 bites.
Here is a definition of structure
typedef struct _sds_ht_instance
{
uint32_t checksum;
char hkey[16];
sds_ht_node *root;
int64_t (*key_cmp_fn)(void *a, void *b);
uint64_t (*key_size_fn)(void *key);
void *(*key_dup_fn)(void *key);
void (*key_free_fn)(void *key);
void *(*value_dup_fn)(void *value);
void (*value_free_fn)(void *value);
} sds_ht_instance;
This structure is always created with malloc. So the beginning of structure is
aligned. "ht_ptr = sds_calloc(sizeof(sds_ht_instance));"
But it is very possible that hkey will not be aligned to 64 bits unless
compiler adds some padding between "checksum" and "hkey".
You might do the trick to change order of members in structure which would
solve problem here. But it still would not solve problem with improperly
aligned input in sds_siphash13.
let say we have following code
int main(void)
{
char *input = strdup("lorem impsum");
char *key = calloc(1, 16);
uint64_t temp;
/* this version would not crash) */
temp = sds_siphash13(input, strlen(input), key);
/* but this version will crash */
temp = sds_siphash13(input + 1, strlen(input +1), key);
return 0;
}
It is really difficult to rely on proper alignment of input.
In theory you could have assert in code which would catch it
also in x86_64 architecture. But it is not nice solution.
The only portable way is to use memcpy to copy data from
array of char to variable/array of uint64_t.
BTW recently I read nice article and it can crash even on x86_64
in case of vector operations. Which compilers try to use as part of
optimisations.
https://pzemtsov.github.io/2016/11/06/bug-story-alignment-on-x86.html
And as you can see in article usage of memcpy is not so bad especially
if compiler uses builtin instead of procedure call.
HTH and sorry for late reply.
LS
___
389-devel mailing list -- 389-devel@lists.fedoraproject.org
To unsubscribe send an email to 389-devel-le...@lists.fedoraproject.org
[389-devel] Re: csiphash on Sparc
On Wed, 2017-10-11 at 13:36 +0200, Lukas Slebodnik wrote:
> On (11/10/17 12:42), Carsten Grzemba wrote:
> >
>
> >
>
> >On 11.10.17 10:54, William Brown wrote:
>
> >>
>
> >> On Tue, 2017-10-10 at 16:28 +0200, Carsten Grzemba wrote:
>
> >> >
>
> >> > On 10.10.17 16:10, William Brown wrote:
>
> >> > >
>
> >> > > On Fri, 2017-10-06 at 10:21 +0200, Carsten Grzemba wrote:
>
> >> > > > Currently the code src/libsds/external/csiphash/csiphash.c do not
> >> > > > work on Sparc.
>
> >> > > > The casting void* or char* to unit64_t* throws Bus-Error.
>
> >> > > >
>
> >> > > > The solution would be to copy the content of the void and char
> >> > > > pointer so that the variabeles are suitably aligned.
>
> >> > > > To prevent have to use malloc: do we know the max of src_sz?
>
> >> > > >
>
> >> > >
>
> >> > >
>
> >> > > What line is this? I assume you are refering to:
>
> >> > >
>
> >> > > https://pagure.io/389-ds-base/blob/master/f/src/libsds/external/csiphash/csiphash.c#_82
>
> >> > >
>
> >> > yes!
>
> >> >
>
> >> > >
>
> >> > > (https://pagure.io/389-ds-base/blob/master/f/src/libsds/external/csiphash/csiphash.c#_82)
>
> >> > >
>
> >> > > No, we can't know the max of src_sz, in theory it could be uint64_t
> >> > > max.
>
> >> > > So this may not be an option.
>
> >> > >
>
> >> > So is malloc for uint64 alignment of the src variable the only option?
>
> >> >
>
> >> > >
>
> >> > >
>
> >> > >
>
> >> > > Are you trying this on a 32bit platform perhaps? What's the arch of the
>
> >> > > machine with the issue?
>
> >> > >
>
> >> > 64bit, Fujitsu M4000
>
> >> > $ isainfo -v
>
> >> > 64-bit sparcv9 applications
>
> >> > fmaf vis2 vis popc
>
> >>
>
> >> I feel like there is something I'm missing here in the problem. What is
>
> >> sizeof(void *) on this platform? I'm assuming 4 or 16 rather than 8
>
> >> bytes? Is this correct?
>
> >>
>
> >no it is 8.
>
> >
>
> >The following programm works on x86 but dumps on Sparc:
>
> >
>
> >
>
> >
>
> >#include
>
> >#include
>
> >
>
> >int func(const void *str, size_t sz, const char key[16]){
>
> > uint64_t *ip = (uint64_t*) str;
>
> > printf ("str: %lx:%lx\n", ip, *ip);
>
> >}
>
> >
>
> >int main()
>
> >{
>
> > char str[25] = "ABCDEFGH12345678";
>
> > char key[16];
>
> >
>
> But following code should work. Please correct me if I am wrong. I didn't
> test.
> char *str = strdup("ABCDEFGH12345678");
> char *key = malloc(16);
>
> yes, function sds_siphash13 is not ideal because it rely on properly alligned
> input data.
>
We are free to change the signature of the function, it's just that I
used this from another open source component (thus why it's slightly
different style wise)
What would you suggest here Lukas?
--
Sincerely,
William Brown
Software Engineer
Red Hat, Australia/Brisbane
signature.asc
Description: This is a digitally signed message part
___
389-devel mailing list -- 389-devel@lists.fedoraproject.org
To unsubscribe send an email to 389-devel-le...@lists.fedoraproject.org
[389-devel] Re: csiphash on Sparc
On (11/10/17 12:42), Carsten Grzemba wrote:
>
>
>On 11.10.17 10:54, William Brown wrote:
>>
>> On Tue, 2017-10-10 at 16:28 +0200, Carsten Grzemba wrote:
>> >
>> > On 10.10.17 16:10, William Brown wrote:
>> > >
>> > > On Fri, 2017-10-06 at 10:21 +0200, Carsten Grzemba wrote:
>> > > > Currently the code src/libsds/external/csiphash/csiphash.c do not work
>> > > > on Sparc.
>> > > > The casting void* or char* to unit64_t* throws Bus-Error.
>> > > >
>> > > > The solution would be to copy the content of the void and char pointer
>> > > > so that the variabeles are suitably aligned.
>> > > > To prevent have to use malloc: do we know the max of src_sz?
>> > > >
>> > >
>> > >
>> > > What line is this? I assume you are refering to:
>> > >
>> > > https://pagure.io/389-ds-base/blob/master/f/src/libsds/external/csiphash/csiphash.c#_82
>> > >
>> > yes!
>> >
>> > >
>> > > (https://pagure.io/389-ds-base/blob/master/f/src/libsds/external/csiphash/csiphash.c#_82)
>> > >
>> > > No, we can't know the max of src_sz, in theory it could be uint64_t max.
>> > > So this may not be an option.
>> > >
>> > So is malloc for uint64 alignment of the src variable the only option?
>> >
>> > >
>> > >
>> > >
>> > > Are you trying this on a 32bit platform perhaps? What's the arch of the
>> > > machine with the issue?
>> > >
>> > 64bit, Fujitsu M4000
>> > $ isainfo -v
>> > 64-bit sparcv9 applications
>> > fmaf vis2 vis popc
>>
>> I feel like there is something I'm missing here in the problem. What is
>> sizeof(void *) on this platform? I'm assuming 4 or 16 rather than 8
>> bytes? Is this correct?
>>
>no it is 8.
>
>The following programm works on x86 but dumps on Sparc:
>
>
>
>#include
>#include
>
>int func(const void *str, size_t sz, const char key[16]){
> uint64_t *ip = (uint64_t*) str;
> printf ("str: %lx:%lx\n", ip, *ip);
>}
>
>int main()
>{
> char str[25] = "ABCDEFGH12345678";
> char key[16];
>
But following code should work. Please correct me if I am wrong. I didn't test.
char *str = strdup("ABCDEFGH12345678");
char *key = malloc(16);
yes, function sds_siphash13 is not ideal because it rely on properly alligned
input data.
LS
___
389-devel mailing list -- 389-devel@lists.fedoraproject.org
To unsubscribe send an email to 389-devel-le...@lists.fedoraproject.org
[389-devel] Re: csiphash on Sparc
On 11.10.17 10:54, William Brown wrote:
>
> On Tue, 2017-10-10 at 16:28 +0200, Carsten Grzemba wrote:
> >
> > On 10.10.17 16:10, William Brown wrote:
> > >
> > > On Fri, 2017-10-06 at 10:21 +0200, Carsten Grzemba wrote:
> > > > Currently the code src/libsds/external/csiphash/csiphash.c do not work
> > > > on Sparc.
> > > > The casting void* or char* to unit64_t* throws Bus-Error.
> > > >
> > > > The solution would be to copy the content of the void and char pointer
> > > > so that the variabeles are suitably aligned.
> > > > To prevent have to use malloc: do we know the max of src_sz?
> > > >
> > >
> > >
> > > What line is this? I assume you are refering to:
> > >
> > > https://pagure.io/389-ds-base/blob/master/f/src/libsds/external/csiphash/csiphash.c#_82
> > >
> > yes!
> >
> > >
> > > (https://pagure.io/389-ds-base/blob/master/f/src/libsds/external/csiphash/csiphash.c#_82)
> > >
> > > No, we can't know the max of src_sz, in theory it could be uint64_t max.
> > > So this may not be an option.
> > >
> > So is malloc for uint64 alignment of the src variable the only option?
> >
> > >
> > >
> > >
> > > Are you trying this on a 32bit platform perhaps? What's the arch of the
> > > machine with the issue?
> > >
> > 64bit, Fujitsu M4000
> > $ isainfo -v
> > 64-bit sparcv9 applications
> > fmaf vis2 vis popc
>
> I feel like there is something I'm missing here in the problem. What is
> sizeof(void *) on this platform? I'm assuming 4 or 16 rather than 8
> bytes? Is this correct?
>
no it is 8.
The following programm works on x86 but dumps on Sparc:
#include
#include
int func(const void *str, size_t sz, const char key[16]){
uint64_t *ip = (uint64_t*) str;
printf ("str: %lx:%lx\n", ip, *ip);
}
int main()
{
char str[25] = "ABCDEFGH12345678";
char key[16];
printf ("sizeof void* %d\n", sizeof(void*));
printf ("sizeof char* %d\n", sizeof(char*));
printf ("sizeof uint64_t* %d\n", sizeof(uint64_t*));
func(str, 12, key);
func(&str[1], 11, key);
}
On x86:
grzemba@unstable11x:~/Tests$ gcc -g -m64 -O3 -fPIC -DPIC -MD -MP size_test.c -o
st_x86
grzemba@unstable11x:~/Tests$ ./st_x86
sizeof void* 8
sizeof char* 8
sizeof uint64_t* 8
str: 80ffbb20:4847464544434241
str: 80ffbb21:3148474645444342
On Sparc:
grzemba@unstable11s:~/Tests$ gcc -g -m64 -O3 -fPIC -DPIC -MD -MP size_test.c -o
st_sparc
grzemba@unstable11s:~/Tests$ ./st_sparc
sizeof void* 8
sizeof char* 8
sizeof uint64_t* 8
str: 77d0:4142434445464748
Bus Error (core dumped)
grzemba@unstable11s:~/Tests$ gdb st_sparc core
GNU gdb (GDB) 7.6
Copyright (C) 2013 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law. Type "show copying"
and "show warranty" for details.
This GDB was configured as "sparc-sun-solaris2.11".
For bug reporting instructions, please see:
[389-devel] Re: csiphash on Sparc
On Tue, 2017-10-10 at 16:28 +0200, Carsten Grzemba wrote: > > On 10.10.17 16:10, William Brown wrote: > > > > On Fri, 2017-10-06 at 10:21 +0200, Carsten Grzemba wrote: > > > Currently the code src/libsds/external/csiphash/csiphash.c do not work on > > > Sparc. > > > The casting void* or char* to unit64_t* throws Bus-Error. > > > > > > The solution would be to copy the content of the void and char pointer so > > > that the variabeles are suitably aligned. > > > To prevent have to use malloc: do we know the max of src_sz? > > > > > > > > > What line is this? I assume you are refering to: > > > > https://pagure.io/389-ds-base/blob/master/f/src/libsds/external/csiphash/csiphash.c#_82 > > > yes! > > > > > (https://pagure.io/389-ds-base/blob/master/f/src/libsds/external/csiphash/csiphash.c#_82) > > > > No, we can't know the max of src_sz, in theory it could be uint64_t max. > > So this may not be an option. > > > So is malloc for uint64 alignment of the src variable the only option? > > > > > > > > > Are you trying this on a 32bit platform perhaps? What's the arch of the > > machine with the issue? > > > 64bit, Fujitsu M4000 > $ isainfo -v > 64-bit sparcv9 applications > fmaf vis2 vis popc I feel like there is something I'm missing here in the problem. What is sizeof(void *) on this platform? I'm assuming 4 or 16 rather than 8 bytes? Is this correct? -- Sincerely, William Brown Software Engineer Red Hat, Australia/Brisbane signature.asc Description: This is a digitally signed message part ___ 389-devel mailing list -- 389-devel@lists.fedoraproject.org To unsubscribe send an email to 389-devel-le...@lists.fedoraproject.org
[389-devel] Re: csiphash on Sparc
On 10.10.17 16:10, William Brown wrote: > > On Fri, 2017-10-06 at 10:21 +0200, Carsten Grzemba wrote: > > Currently the code src/libsds/external/csiphash/csiphash.c do not work on > > Sparc. > > The casting void* or char* to unit64_t* throws Bus-Error. > > > > The solution would be to copy the content of the void and char pointer so > > that the variabeles are suitably aligned. > > To prevent have to use malloc: do we know the max of src_sz? > > > > > What line is this? I assume you are refering to: > > https://pagure.io/389-ds-base/blob/master/f/src/libsds/external/csiphash/csiphash.c#_82 > yes! > > (https://pagure.io/389-ds-base/blob/master/f/src/libsds/external/csiphash/csiphash.c#_82) > > No, we can't know the max of src_sz, in theory it could be uint64_t max. > So this may not be an option. > So is malloc for uint64 alignment of the src variable the only option? > > > > Are you trying this on a 32bit platform perhaps? What's the arch of the > machine with the issue? > 64bit, Fujitsu M4000 $ isainfo -v 64-bit sparcv9 applications fmaf vis2 vis popc Carsten ___ 389-devel mailing list -- 389-devel@lists.fedoraproject.org To unsubscribe send an email to 389-devel-le...@lists.fedoraproject.org
[389-devel] Re: csiphash on Sparc
On Fri, 2017-10-06 at 10:21 +0200, Carsten Grzemba wrote: > Currently the code src/libsds/external/csiphash/csiphash.c do not work on > Sparc. > The casting void* or char* to unit64_t* throws Bus-Error. > > The solution would be to copy the content of the void and char pointer so > that the variabeles are suitably aligned. > To prevent have to use malloc: do we know the max of src_sz? > What line is this? I assume you are refering to: https://pagure.io/389-ds-base/blob/master/f/src/libsds/external/csiphash/csiphash.c#_82 No, we can't know the max of src_sz, in theory it could be uint64_t max. So this may not be an option. Are you trying this on a 32bit platform perhaps? What's the arch of the machine with the issue? -- Sincerely, William Brown Software Engineer Red Hat, Australia/Brisbane signature.asc Description: This is a digitally signed message part ___ 389-devel mailing list -- 389-devel@lists.fedoraproject.org To unsubscribe send an email to 389-devel-le...@lists.fedoraproject.org
