Re: [systemd-devel] [PATCH] fix strict aliasing violations in src/udev/udev-builtin-usb_id.c

2015-04-12 Thread Lennart Poettering 
On Tue, 10.03.15 19:13, Shawn Landden (sh...@churchofgit.com) wrote:

 @@ -153,15 +154,15 @@ static int dev_if_packed_info(struct udev_device *dev, 
 char *ifs_str, size_t len
  int pos = 0;
  unsigned strpos = 0;
  struct usb_interface_descriptor {
 -u_int8_tbLength;
 -u_int8_tbDescriptorType;
 -u_int8_tbInterfaceNumber;
 -u_int8_tbAlternateSetting;
 -u_int8_tbNumEndpoints;
 -u_int8_tbInterfaceClass;
 -u_int8_tbInterfaceSubClass;
 -u_int8_tbInterfaceProtocol;
 -u_int8_tiInterface;
 +uint8_tbLength;
 +uint8_tbDescriptorType;
 +uint8_tbInterfaceNumber;
 +uint8_tbAlternateSetting;
 +uint8_tbNumEndpoints;
 +uint8_tbInterfaceClass;
 +uint8_tbInterfaceSubClass;
 +uint8_tbInterfaceProtocol;
 +uint8_tiInterface;
  } __attribute__((packed));

This part has already been fixed with a different patch.

  
  if (asprintf(filename, %s/descriptors, 
 udev_device_get_syspath(dev))  0)
 @@ -179,21 +180,20 @@ static int dev_if_packed_info(struct udev_device *dev, 
 char *ifs_str, size_t len
  
  ifs_str[0] = '\0';
  while (pos  size  strpos+7  len-2) {
 -struct usb_interface_descriptor *desc;
 +char *desc = buf[pos];
  char if_str[8];
  
 -desc = (struct usb_interface_descriptor *) buf[pos];
 -if (desc-bLength  3)
 +if (desc[offsetof(struct usb_interface_descriptor, bLength)] 
  3)
  break;
 -pos += desc-bLength;
 +pos += desc[offsetof(struct usb_interface_descriptor, 
 bLength)];
  
 -if (desc-bDescriptorType != USB_DT_INTERFACE)
 +if (desc[offsetof(struct usb_interface_descriptor, 
 bDescriptorType)] != USB_DT_INTERFACE)
  continue;
  
  if (snprintf(if_str, 8, :%02x%02x%02x,
 - desc-bInterfaceClass,
 - desc-bInterfaceSubClass,
 - desc-bInterfaceProtocol) != 7)
 + desc[offsetof(struct usb_interface_descriptor, 
 bInterfaceClass)],
 + desc[offsetof(struct usb_interface_descriptor, 
 bInterfaceSubClass)],
 + desc[offsetof(struct usb_interface_descriptor, 
 bInterfaceProtocol)]) != 7)
  continue;

This call doesn't look pretty. I don#t really care too much about the
strict aliasing issues, but if you really want to fix this, then this
should probably be done with a union, like the other cases.

Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] fix strict aliasing violations in src/udev/udev-builtin-usb_id.c

2015-03-08 Thread Lennart Poettering 
On Thu, 05.03.15 04:58, Zbigniew Jędrzejewski-Szmek (zbys...@in.waw.pl) wrote:

  +uint8_tbLength;
  +uint8_tbDescriptorType;
  +uint8_tbInterfaceNumber;
  +uint8_tbAlternateSetting;
  +uint8_tbNumEndpoints;
  +uint8_tbInterfaceClass;
  +uint8_tbInterfaceSubClass;
  +uint8_tbInterfaceProtocol;
  +uint8_tiInterface;
   } __attribute__((packed));
   
   if (asprintf(filename, %s/descriptors, 
  udev_device_get_syspath(dev))  0)
  @@ -179,21 +180,21 @@ static int dev_if_packed_info(struct udev_device 
  *dev, char *ifs_str, size_t len
   
   ifs_str[0] = '\0';
   while (pos  size  strpos+7  len-2) {
  -struct usb_interface_descriptor *desc;
  +struct usb_interface_descriptor desc;
   char if_str[8];
   
  -desc = (struct usb_interface_descriptor *) buf[pos];
  -if (desc-bLength  3)
  +memcpy(desc, buf[pos], sizeof(desc));
 Copying it seems suboptimal. But is this actually an aliasing
 violation? buf is a char array, and [1] says: a character type
 may alias any other type.
 
 [1] 
 https://gcc.gnu.org/onlinedocs/gcc-4.6.1/gcc/Optimize-Options.html#index-fstrict_002daliasing-825

Also, I greatly prefer using unions for these things, to make the
aliasing explicit, rather than copying things.

Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] fix strict aliasing violations in src/udev/udev-builtin-usb_id.c

2015-03-04 Thread Zbigniew Jędrzejewski-Szmek 
On Tue, Mar 03, 2015 at 04:21:30PM -0800, Shawn Landden wrote:
 also switch to inttypes.h
 ---
  src/udev/udev-builtin-usb_id.c | 35 ++-
  1 file changed, 18 insertions(+), 17 deletions(-)
 
 diff --git a/src/udev/udev-builtin-usb_id.c b/src/udev/udev-builtin-usb_id.c
 index ab0d96e..0223421 100644
 --- a/src/udev/udev-builtin-usb_id.c
 +++ b/src/udev/udev-builtin-usb_id.c
 @@ -28,6 +28,7 @@
  #include ctype.h
  #include fcntl.h
  #include errno.h
 +#include inttypes.h
  
  #include udev.h
  
 @@ -153,15 +154,15 @@ static int dev_if_packed_info(struct udev_device *dev, 
 char *ifs_str, size_t len
  int pos = 0;
  unsigned strpos = 0;
  struct usb_interface_descriptor {
 -u_int8_tbLength;
 -u_int8_tbDescriptorType;
 -u_int8_tbInterfaceNumber;
 -u_int8_tbAlternateSetting;
 -u_int8_tbNumEndpoints;
 -u_int8_tbInterfaceClass;
 -u_int8_tbInterfaceSubClass;
 -u_int8_tbInterfaceProtocol;
 -u_int8_tiInterface;
 +uint8_tbLength;
 +uint8_tbDescriptorType;
 +uint8_tbInterfaceNumber;
 +uint8_tbAlternateSetting;
 +uint8_tbNumEndpoints;
 +uint8_tbInterfaceClass;
 +uint8_tbInterfaceSubClass;
 +uint8_tbInterfaceProtocol;
 +uint8_tiInterface;
  } __attribute__((packed));
  
  if (asprintf(filename, %s/descriptors, 
 udev_device_get_syspath(dev))  0)
 @@ -179,21 +180,21 @@ static int dev_if_packed_info(struct udev_device *dev, 
 char *ifs_str, size_t len
  
  ifs_str[0] = '\0';
  while (pos  size  strpos+7  len-2) {
 -struct usb_interface_descriptor *desc;
 +struct usb_interface_descriptor desc;
  char if_str[8];
  
 -desc = (struct usb_interface_descriptor *) buf[pos];
 -if (desc-bLength  3)
 +memcpy(desc, buf[pos], sizeof(desc));
Copying it seems suboptimal. But is this actually an aliasing
violation? buf is a char array, and [1] says: a character type
may alias any other type.

[1] 
https://gcc.gnu.org/onlinedocs/gcc-4.6.1/gcc/Optimize-Options.html#index-fstrict_002daliasing-825

Zbyszek
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] fix strict aliasing violations in src/udev/udev-builtin-usb_id.c

2015-03-04 Thread Shawn Landden 
On Wed, Mar 4, 2015 at 7:58 PM, Zbigniew Jędrzejewski-Szmek 
zbys...@in.waw.pl wrote:

 On Tue, Mar 03, 2015 at 04:21:30PM -0800, Shawn Landden wrote:
  also switch to inttypes.h
  ---
   src/udev/udev-builtin-usb_id.c | 35 ++-
   1 file changed, 18 insertions(+), 17 deletions(-)
 
  diff --git a/src/udev/udev-builtin-usb_id.c
 b/src/udev/udev-builtin-usb_id.c
  index ab0d96e..0223421 100644
  --- a/src/udev/udev-builtin-usb_id.c
  +++ b/src/udev/udev-builtin-usb_id.c
  @@ -28,6 +28,7 @@
   #include ctype.h
   #include fcntl.h
   #include errno.h
  +#include inttypes.h
 
   #include udev.h
 
  @@ -153,15 +154,15 @@ static int dev_if_packed_info(struct udev_device
 *dev, char *ifs_str, size_t len
   int pos = 0;
   unsigned strpos = 0;
   struct usb_interface_descriptor {
  -u_int8_tbLength;
  -u_int8_tbDescriptorType;
  -u_int8_tbInterfaceNumber;
  -u_int8_tbAlternateSetting;
  -u_int8_tbNumEndpoints;
  -u_int8_tbInterfaceClass;
  -u_int8_tbInterfaceSubClass;
  -u_int8_tbInterfaceProtocol;
  -u_int8_tiInterface;
  +uint8_tbLength;
  +uint8_tbDescriptorType;
  +uint8_tbInterfaceNumber;
  +uint8_tbAlternateSetting;
  +uint8_tbNumEndpoints;
  +uint8_tbInterfaceClass;
  +uint8_tbInterfaceSubClass;
  +uint8_tbInterfaceProtocol;
  +uint8_tiInterface;
   } __attribute__((packed));
 
   if (asprintf(filename, %s/descriptors,
 udev_device_get_syspath(dev))  0)
  @@ -179,21 +180,21 @@ static int dev_if_packed_info(struct udev_device
 *dev, char *ifs_str, size_t len
 
   ifs_str[0] = '\0';
   while (pos  size  strpos+7  len-2) {
  -struct usb_interface_descriptor *desc;
  +struct usb_interface_descriptor desc;
   char if_str[8];
 
  -desc = (struct usb_interface_descriptor *) buf[pos];
  -if (desc-bLength  3)
  +memcpy(desc, buf[pos], sizeof(desc));
 Copying it seems suboptimal. But is this actually an aliasing
 violation? buf is a char array, and [1] says: a character type
 may alias any other type.

 Common misunderstanding about strict aliasing. if accessing as char * yes,
but not the other way around. See the C11 standard which makes it clear
(don't have page number off the top of my head...)

 [1]
 https://gcc.gnu.org/onlinedocs/gcc-4.6.1/gcc/Optimize-Options.html#index-fstrict_002daliasing-825

 Zbyszek
 ___
 systemd-devel mailing list
 systemd-devel@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/systemd-devel




-- 
Shawn Landden
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] fix strict aliasing violations in src/udev/udev-builtin-usb_id.c

2015-03-04 Thread Shawn Landden 
Oh wait, I c, yes I had same question.

On Wed, Mar 4, 2015 at 8:07 PM, Shawn Landden sh...@churchofgit.com wrote:

 On Wed, Mar 4, 2015 at 7:58 PM, Zbigniew Jędrzejewski-Szmek 
 zbys...@in.waw.pl wrote:

 On Tue, Mar 03, 2015 at 04:21:30PM -0800, Shawn Landden wrote:
  also switch to inttypes.h
  ---
   src/udev/udev-builtin-usb_id.c | 35 ++-
   1 file changed, 18 insertions(+), 17 deletions(-)
 
  diff --git a/src/udev/udev-builtin-usb_id.c
 b/src/udev/udev-builtin-usb_id.c
  index ab0d96e..0223421 100644
  --- a/src/udev/udev-builtin-usb_id.c
  +++ b/src/udev/udev-builtin-usb_id.c
  @@ -28,6 +28,7 @@
   #include ctype.h
   #include fcntl.h
   #include errno.h
  +#include inttypes.h
 
   #include udev.h
 
  @@ -153,15 +154,15 @@ static int dev_if_packed_info(struct udev_device
 *dev, char *ifs_str, size_t len
   int pos = 0;
   unsigned strpos = 0;
   struct usb_interface_descriptor {
  -u_int8_tbLength;
  -u_int8_tbDescriptorType;
  -u_int8_tbInterfaceNumber;
  -u_int8_tbAlternateSetting;
  -u_int8_tbNumEndpoints;
  -u_int8_tbInterfaceClass;
  -u_int8_tbInterfaceSubClass;
  -u_int8_tbInterfaceProtocol;
  -u_int8_tiInterface;
  +uint8_tbLength;
  +uint8_tbDescriptorType;
  +uint8_tbInterfaceNumber;
  +uint8_tbAlternateSetting;
  +uint8_tbNumEndpoints;
  +uint8_tbInterfaceClass;
  +uint8_tbInterfaceSubClass;
  +uint8_tbInterfaceProtocol;
  +uint8_tiInterface;
   } __attribute__((packed));
 
   if (asprintf(filename, %s/descriptors,
 udev_device_get_syspath(dev))  0)
  @@ -179,21 +180,21 @@ static int dev_if_packed_info(struct udev_device
 *dev, char *ifs_str, size_t len
 
   ifs_str[0] = '\0';
   while (pos  size  strpos+7  len-2) {
  -struct usb_interface_descriptor *desc;
  +struct usb_interface_descriptor desc;
   char if_str[8];
 
  -desc = (struct usb_interface_descriptor *) buf[pos];
  -if (desc-bLength  3)
  +memcpy(desc, buf[pos], sizeof(desc));
 Copying it seems suboptimal. But is this actually an aliasing
 violation? buf is a char array, and [1] says: a character type
 may alias any other type.

 Common misunderstanding about strict aliasing. if accessing as char *
 yes, but not the other way around. See the C11 standard which makes it
 clear (don't have page number off the top of my head...)

 [1]
 https://gcc.gnu.org/onlinedocs/gcc-4.6.1/gcc/Optimize-Options.html#index-fstrict_002daliasing-825

 Zbyszek
 ___
 systemd-devel mailing list
 systemd-devel@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/systemd-devel




 --
 Shawn Landden




-- 
Shawn Landden
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel