Re: [ovs-dev] [PATCH 4/5] test-hash: Fix unaligned pointers.

[Joe Stringer]

On 25 May 2017 at 13:54, Ben Pfaff  wrote:
> On Thu, May 25, 2017 at 01:18:21PM -0700, Joe Stringer wrote:
>> On 25 May 2017 at 10:42, Ben Pfaff  wrote:
>> > On Tue, May 23, 2017 at 04:02:15PM -0700, Joe Stringer wrote:
>> >> Clang 4.0 complains:
>> >>
>> >> ../tests/test-hash.c:160:16: error: taking address of packed member 'b' of
>> >> class or structure 'offset_ovs_u128' may result in an unaligned pointer 
>> >> value
>> >>   [-Werror,-Waddress-of-packed-member]
>> >> in0 = _data.b;
>> >>
>> >> Rework the 128-bit hash test to have a separate function for setting
>> >> bits in the 32-bit offset u128 structure.
>> >>
>> >> Signed-off-by: Joe Stringer 
>> >
>> > How about something like this, to reduce code duplication?  I have not
>> > tested it with Clang 4.0.
>> >
>> > diff --git a/tests/test-hash.c b/tests/test-hash.c
>> > index d1beead36ed5..f02f0218c71f 100644
>> > --- a/tests/test-hash.c
>> > +++ b/tests/test-hash.c
>> > @@ -153,14 +153,13 @@ check_hash_bytes128(void (*hash)(const void *, 
>> > size_t, uint32_t, ovs_u128 *),
>> >  OVS_PACKED(struct offset_ovs_u128 {
>> >  uint32_t a;
>> >  ovs_u128 b;
>> > -}) in0_data;
>> > -ovs_u128 *in0, in1;
>> > +}) in0;
>> > +ovs_u128 in1;
>> >  ovs_u128 out0, out1;
>> >
>> > -in0 = _data.b;
>> > -set_bit128(in0, i, n_bits);
>> >  set_bit128(, i, n_bits);
>> > -hash(in0, sizeof(ovs_u128), 0, );
>> > +in0.b = in1;
>> > +hash(, sizeof(ovs_u128), 0, );
>> >  hash(, sizeof(ovs_u128), 0, );
>> >  if (!ovs_u128_equals(out0, out1)) {
>> >  printf("%s hash not the same for non-64 aligned data "
>>
>> Thanks, this looks like a much better approach and it satisfies clang
>> 4.0. Will you propose this formally or shall I? The 256B version needs
>> a slight variation on this as well.
>
> I was hoping that you would propose it formally.

OK, I can do that.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 4/5] test-hash: Fix unaligned pointers.

[Ben Pfaff]

On Thu, May 25, 2017 at 01:18:21PM -0700, Joe Stringer wrote:
> On 25 May 2017 at 10:42, Ben Pfaff  wrote:
> > On Tue, May 23, 2017 at 04:02:15PM -0700, Joe Stringer wrote:
> >> Clang 4.0 complains:
> >>
> >> ../tests/test-hash.c:160:16: error: taking address of packed member 'b' of
> >> class or structure 'offset_ovs_u128' may result in an unaligned pointer 
> >> value
> >>   [-Werror,-Waddress-of-packed-member]
> >> in0 = _data.b;
> >>
> >> Rework the 128-bit hash test to have a separate function for setting
> >> bits in the 32-bit offset u128 structure.
> >>
> >> Signed-off-by: Joe Stringer 
> >
> > How about something like this, to reduce code duplication?  I have not
> > tested it with Clang 4.0.
> >
> > diff --git a/tests/test-hash.c b/tests/test-hash.c
> > index d1beead36ed5..f02f0218c71f 100644
> > --- a/tests/test-hash.c
> > +++ b/tests/test-hash.c
> > @@ -153,14 +153,13 @@ check_hash_bytes128(void (*hash)(const void *, 
> > size_t, uint32_t, ovs_u128 *),
> >  OVS_PACKED(struct offset_ovs_u128 {
> >  uint32_t a;
> >  ovs_u128 b;
> > -}) in0_data;
> > -ovs_u128 *in0, in1;
> > +}) in0;
> > +ovs_u128 in1;
> >  ovs_u128 out0, out1;
> >
> > -in0 = _data.b;
> > -set_bit128(in0, i, n_bits);
> >  set_bit128(, i, n_bits);
> > -hash(in0, sizeof(ovs_u128), 0, );
> > +in0.b = in1;
> > +hash(, sizeof(ovs_u128), 0, );
> >  hash(, sizeof(ovs_u128), 0, );
> >  if (!ovs_u128_equals(out0, out1)) {
> >  printf("%s hash not the same for non-64 aligned data "
> 
> Thanks, this looks like a much better approach and it satisfies clang
> 4.0. Will you propose this formally or shall I? The 256B version needs
> a slight variation on this as well.

I was hoping that you would propose it formally.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 4/5] test-hash: Fix unaligned pointers.

[Joe Stringer]

On 25 May 2017 at 10:42, Ben Pfaff  wrote:
> On Tue, May 23, 2017 at 04:02:15PM -0700, Joe Stringer wrote:
>> Clang 4.0 complains:
>>
>> ../tests/test-hash.c:160:16: error: taking address of packed member 'b' of
>> class or structure 'offset_ovs_u128' may result in an unaligned pointer value
>>   [-Werror,-Waddress-of-packed-member]
>> in0 = _data.b;
>>
>> Rework the 128-bit hash test to have a separate function for setting
>> bits in the 32-bit offset u128 structure.
>>
>> Signed-off-by: Joe Stringer 
>
> How about something like this, to reduce code duplication?  I have not
> tested it with Clang 4.0.
>
> diff --git a/tests/test-hash.c b/tests/test-hash.c
> index d1beead36ed5..f02f0218c71f 100644
> --- a/tests/test-hash.c
> +++ b/tests/test-hash.c
> @@ -153,14 +153,13 @@ check_hash_bytes128(void (*hash)(const void *, size_t, 
> uint32_t, ovs_u128 *),
>  OVS_PACKED(struct offset_ovs_u128 {
>  uint32_t a;
>  ovs_u128 b;
> -}) in0_data;
> -ovs_u128 *in0, in1;
> +}) in0;
> +ovs_u128 in1;
>  ovs_u128 out0, out1;
>
> -in0 = _data.b;
> -set_bit128(in0, i, n_bits);
>  set_bit128(, i, n_bits);
> -hash(in0, sizeof(ovs_u128), 0, );
> +in0.b = in1;
> +hash(, sizeof(ovs_u128), 0, );
>  hash(, sizeof(ovs_u128), 0, );
>  if (!ovs_u128_equals(out0, out1)) {
>  printf("%s hash not the same for non-64 aligned data "

Thanks, this looks like a much better approach and it satisfies clang
4.0. Will you propose this formally or shall I? The 256B version needs
a slight variation on this as well.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 4/5] test-hash: Fix unaligned pointers.

[Ben Pfaff]

On Tue, May 23, 2017 at 04:02:15PM -0700, Joe Stringer wrote:
> Clang 4.0 complains:
> 
> ../tests/test-hash.c:160:16: error: taking address of packed member 'b' of
> class or structure 'offset_ovs_u128' may result in an unaligned pointer value
>   [-Werror,-Waddress-of-packed-member]
> in0 = _data.b;
> 
> Rework the 128-bit hash test to have a separate function for setting
> bits in the 32-bit offset u128 structure.
> 
> Signed-off-by: Joe Stringer 

How about something like this, to reduce code duplication?  I have not
tested it with Clang 4.0.

diff --git a/tests/test-hash.c b/tests/test-hash.c
index d1beead36ed5..f02f0218c71f 100644
--- a/tests/test-hash.c
+++ b/tests/test-hash.c
@@ -153,14 +153,13 @@ check_hash_bytes128(void (*hash)(const void *, size_t, 
uint32_t, ovs_u128 *),
 OVS_PACKED(struct offset_ovs_u128 {
 uint32_t a;
 ovs_u128 b;
-}) in0_data;
-ovs_u128 *in0, in1;
+}) in0;
+ovs_u128 in1;
 ovs_u128 out0, out1;
 
-in0 = _data.b;
-set_bit128(in0, i, n_bits);
 set_bit128(, i, n_bits);
-hash(in0, sizeof(ovs_u128), 0, );
+in0.b = in1;
+hash(, sizeof(ovs_u128), 0, );
 hash(, sizeof(ovs_u128), 0, );
 if (!ovs_u128_equals(out0, out1)) {
 printf("%s hash not the same for non-64 aligned data "
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH 4/5] test-hash: Fix unaligned pointers.

[Joe Stringer]

Clang 4.0 complains:

../tests/test-hash.c:160:16: error: taking address of packed member 'b' of
class or structure 'offset_ovs_u128' may result in an unaligned pointer value
  [-Werror,-Waddress-of-packed-member]
in0 = _data.b;

Rework the 128-bit hash test to have a separate function for setting
bits in the 32-bit offset u128 structure.

Signed-off-by: Joe Stringer 
---
 tests/test-hash.c | 36 
 1 file changed, 28 insertions(+), 8 deletions(-)

diff --git a/tests/test-hash.c b/tests/test-hash.c
index a20c87fad6a0..704e4bc72ff9 100644
--- a/tests/test-hash.c
+++ b/tests/test-hash.c
@@ -25,6 +25,12 @@
 #include "jhash.h"
 #include "ovstest.h"
 
+OVS_PACKED(struct offset_ovs_u128
+{
+uint32_t a;
+ovs_u128 b;
+});
+
 static void
 set_bit(uint32_t array[3], int bit)
 {
@@ -51,6 +57,21 @@ set_bit128(ovs_u128 *values, int bit, int n_bits)
 }
 }
 
+static void
+set_bit128_unaligned(struct offset_ovs_u128 *values, int bit, int n_bits)
+{
+int b = bit % 128;
+
+assert(bit >= 0 && bit < 2048);
+memset(values, 0, n_bits/8 + sizeof(values->a));
+
+if (b < 64) {
+values->b.u64.lo = UINT64_C(1) << (b % 64);
+} else {
+values->b.u64.hi = UINT64_C(1) << (b % 64);
+}
+}
+
 static uint64_t
 get_range128(ovs_u128 *value, int ofs, uint64_t mask)
 {
@@ -149,17 +170,16 @@ check_hash_bytes128(void (*hash)(const void *, size_t, 
uint32_t, ovs_u128 *),
 int i, j;
 
 for (i = 0; i < n_bits; i++) {
-OVS_PACKED(struct offset_ovs_u128 {
-uint32_t a;
-ovs_u128 b;
-}) in0_data;
-ovs_u128 *in0, in1;
+struct offset_ovs_u128 in0;
 ovs_u128 out0, out1;
+uint32_t *in0p;
+ovs_u128 in1;
 
-in0 = _data.b;
-set_bit128(in0, i, n_bits);
+in0p = [0];
+set_bit128_unaligned(, i, n_bits);
 set_bit128(, i, n_bits);
-hash(in0, sizeof(ovs_u128), 0, );
+assert(ovs_u128_equals(in0.b, in1));
+hash(in0p, sizeof(ovs_u128), 0, );
 hash(, sizeof(ovs_u128), 0, );
 if (!ovs_u128_equals(out0, out1)) {
 printf("%s hash not the same for non-64 aligned data "
-- 
2.12.2

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev