| Message ID | 1470256668-21374-1-git-send-email-thuth@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, Aug 03, 2016 at 10:37:48PM +0200, Thomas Huth wrote: > With my version of GCC (v4.8.5 - Advance-Toolchain 7.0) there are > currently two warnings when compiling sloffs.c: > > sloffs.c: In function ‘sloffs_dump’: > sloffs.c:437:6: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing] > printf("%04x", be16_to_cpu(*(uint16_t *)(header->date + 2))); > ^ > sloffs.c:449:6: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing] > printf("%04x", be16_to_cpu(*(uint16_t *)(header->mdate + 2))); > ^ > > These can be easily fixed by accessing the memory byte by byte instead of > casting the pointer to (uint16_t *). And while we're at it, let's also > simplify the code a little bit by consolidating the date print code > into a separate function which can be used by the two spots that print > a date. > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- I do not see this warning with gcc 6.1.1 but I also see it with 4.8.5 and with this patch it is fixed. Reviewed-by: Adrian Reber <areber@redhat.com> > diff --git a/tools/sloffs.c b/tools/sloffs.c > index 91a23c3..764c956 100644 > --- a/tools/sloffs.c > +++ b/tools/sloffs.c > @@ -405,6 +405,19 @@ sloffs_append(const int file, const char *name, const char *dest) > close(out); > } > > +static void print_header_date(void *dptr) > +{ > + uint8_t *date = dptr; > + > + if (*(uint64_t *)dptr != 0) { > + printf("%02x%02x-%02x-%02x %02x:%02x", date[2], date[3], > + date[4], date[5], date[6], date[7]); > + } else { > + printf("N/A"); > + } > + > +} > + > static void > sloffs_dump(const int fd) > { > @@ -413,7 +426,6 @@ sloffs_dump(const int fd) > struct sloffs file; > int i; > uint64_t crc; > - uint64_t *datetmp; > uint64_t header_len; > > header = sloffs_header(fd); > @@ -432,28 +444,10 @@ sloffs_dump(const int fd) > /* there is a bug in the date position; > * it should be at header->date, but it is at (header->date + 2) */ > printf(" Build Date : "); > - datetmp = (void *)header->date; > - if (be64_to_cpu(*datetmp)) { > - printf("%04x", be16_to_cpu(*(uint16_t *)(header->date + 2))); > - printf("-%02x", *(uint8_t *)(header->date + 4)); > - printf("-%02x", *(uint8_t *)(header->date + 5)); > - printf(" %02x:", *(uint8_t *)(header->date + 6)); > - printf("%02x", *(uint8_t *)(header->date + 7)); > - } else { > - printf("N/A"); > - } > + print_header_date(header->date); > printf("\n"); > printf(" Modify Date : "); > - datetmp = (void *)header->mdate; > - if (be64_to_cpu(*datetmp)) { > - printf("%04x", be16_to_cpu(*(uint16_t *)(header->mdate + 2))); > - printf("-%02x", *(uint8_t *)(header->mdate + 4)); > - printf("-%02x", *(uint8_t *)(header->mdate + 5)); > - printf(" %02x:", *(uint8_t *)(header->mdate + 6)); > - printf("%02x", *(uint8_t *)(header->mdate + 7)); > - } else { > - printf("N/A"); > - } > + print_header_date(header->mdate); > printf("\n"); > printf(" Image Length: %ld", be64_to_cpu(header->flashlen)); > printf(" (0x%lx) bytes\n", be64_to_cpu(header->flashlen)); > -- > 1.8.3.1 >
On Fri, Aug 05, 2016 at 10:48:18AM +0200, Adrian Reber wrote: > > +static void print_header_date(void *dptr) > > +{ > > + uint8_t *date = dptr; > > + > > + if (*(uint64_t *)dptr != 0) { This is the exact same problem though. Should be something like if (date[0] || date[1] || date[2] || date[3] || date[4] || date[5] || date[6] || date[7]) > > + printf("%02x%02x-%02x-%02x %02x:%02x", date[2], date[3], > > + date[4], date[5], date[6], date[7]); > > + } else { > > + printf("N/A"); > > + } > > + > > +} Segher
On 08.08.2016 03:30, Segher Boessenkool wrote: > On Fri, Aug 05, 2016 at 10:48:18AM +0200, Adrian Reber wrote: >>> +static void print_header_date(void *dptr) >>> +{ >>> + uint8_t *date = dptr; >>> + >>> + if (*(uint64_t *)dptr != 0) { > > This is the exact same problem though. Should be something like > > if (date[0] || date[1] || date[2] || date[3] > || date[4] || date[5] || date[6] || date[7]) I'm now type-punning dptr here, not date, so this time it's a void pointer instead of a char array ... and I thought type-punning a void pointer would be OK since they are considered to alias any other kind of data? I might be wrong here, though, these aliasing problems always cause a headache for me. In the end, we just might want to add -fno-strict-aliasing to the HOSTCFLAGS, just like we already did it in make.rules for the normal CFLAGS, and call it a day. (But IMHO it makes sense to include this patch here anyway since it consolidates the date printing a little bit) Thomas
On Mon, Aug 08, 2016 at 08:38:33AM +0200, Thomas Huth wrote: > On 08.08.2016 03:30, Segher Boessenkool wrote: > > On Fri, Aug 05, 2016 at 10:48:18AM +0200, Adrian Reber wrote: > >>> +static void print_header_date(void *dptr) > >>> +{ > >>> + uint8_t *date = dptr; > >>> + > >>> + if (*(uint64_t *)dptr != 0) { > > > > This is the exact same problem though. Should be something like > > > > if (date[0] || date[1] || date[2] || date[3] > > || date[4] || date[5] || date[6] || date[7]) > > I'm now type-punning dptr here, not date, so this time it's a void > pointer instead of a char array ... and I thought type-punning a void > pointer would be OK since they are considered to alias any other kind of > data? I might be wrong here, though, these aliasing problems always > cause a headache for me. That is not how it works. The pointer type does not matter; it is perfectly legal to cast a pointer. What is not okay is accessing something with a type not compatible with its effective type (ignoring signedness), unless the access is character type. "Effective type" usually is just the declared type of the datum. > In the end, we just might want to add -fno-strict-aliasing to the > HOSTCFLAGS, just like we already did it in make.rules for the normal > CFLAGS, and call it a day. Or you could fix the problems. SLOF used to work with -fstrict-aliasing, and it was a nice performance win. Segher
On 08.08.2016 10:31, Segher Boessenkool wrote: > On Mon, Aug 08, 2016 at 08:38:33AM +0200, Thomas Huth wrote: >> On 08.08.2016 03:30, Segher Boessenkool wrote: >>> On Fri, Aug 05, 2016 at 10:48:18AM +0200, Adrian Reber wrote: >>>>> +static void print_header_date(void *dptr) >>>>> +{ >>>>> + uint8_t *date = dptr; >>>>> + >>>>> + if (*(uint64_t *)dptr != 0) { >>> >>> This is the exact same problem though. Should be something like >>> >>> if (date[0] || date[1] || date[2] || date[3] >>> || date[4] || date[5] || date[6] || date[7]) >> >> I'm now type-punning dptr here, not date, so this time it's a void >> pointer instead of a char array ... and I thought type-punning a void >> pointer would be OK since they are considered to alias any other kind of >> data? I might be wrong here, though, these aliasing problems always >> cause a headache for me. > > That is not how it works. The pointer type does not matter; it is > perfectly legal to cast a pointer. What is not okay is accessing > something with a type not compatible with its effective type (ignoring > signedness), unless the access is character type. "Effective type" > usually is just the declared type of the datum. Ouch, OK, I got it now. I'll send a new version of my patch... >> In the end, we just might want to add -fno-strict-aliasing to the >> HOSTCFLAGS, just like we already did it in make.rules for the normal >> CFLAGS, and call it a day. > > Or you could fix the problems. SLOF used to work with -fstrict-aliasing, > and it was a nice performance win. That's a question for Nikunj who committed that change a couple of years ago ... (see http://git.qemu.org/?p=SLOF.git;a=commitdiff;h=7eca6a5e56f468) Thomas
Thomas Huth <thuth@redhat.com> writes: > On 08.08.2016 10:31, Segher Boessenkool wrote: >> On Mon, Aug 08, 2016 at 08:38:33AM +0200, Thomas Huth wrote: >>> On 08.08.2016 03:30, Segher Boessenkool wrote: >>>> On Fri, Aug 05, 2016 at 10:48:18AM +0200, Adrian Reber wrote: >>>>>> +static void print_header_date(void *dptr) >>>>>> +{ >>>>>> + uint8_t *date = dptr; >>>>>> + >>>>>> + if (*(uint64_t *)dptr != 0) { >>>> >>>> This is the exact same problem though. Should be something like >>>> >>>> if (date[0] || date[1] || date[2] || date[3] >>>> || date[4] || date[5] || date[6] || date[7]) >>> >>> I'm now type-punning dptr here, not date, so this time it's a void >>> pointer instead of a char array ... and I thought type-punning a void >>> pointer would be OK since they are considered to alias any other kind of >>> data? I might be wrong here, though, these aliasing problems always >>> cause a headache for me. >> >> That is not how it works. The pointer type does not matter; it is >> perfectly legal to cast a pointer. What is not okay is accessing >> something with a type not compatible with its effective type (ignoring >> signedness), unless the access is character type. "Effective type" >> usually is just the declared type of the datum. > > Ouch, OK, I got it now. I'll send a new version of my patch... > >>> In the end, we just might want to add -fno-strict-aliasing to the >>> HOSTCFLAGS, just like we already did it in make.rules for the normal >>> CFLAGS, and call it a day. >> >> Or you could fix the problems. SLOF used to work with -fstrict-aliasing, >> and it was a nice performance win. > > That's a question for Nikunj who committed that change a couple of years > ago ... > (see http://git.qemu.org/?p=SLOF.git;a=commitdiff;h=7eca6a5e56f468) I remember that we had a discussion on this and BenH had suggested that we cannot re-audit all the code for correctness with strict-aliasing. So disable it. Regards, Nikunj
On Mon, Aug 08, 2016 at 02:45:35PM +0530, Nikunj A Dadhania wrote: > >>> In the end, we just might want to add -fno-strict-aliasing to the > >>> HOSTCFLAGS, just like we already did it in make.rules for the normal > >>> CFLAGS, and call it a day. > >> > >> Or you could fix the problems. SLOF used to work with -fstrict-aliasing, > >> and it was a nice performance win. > > > > That's a question for Nikunj who committed that change a couple of years > > ago ... > > (see http://git.qemu.org/?p=SLOF.git;a=commitdiff;h=7eca6a5e56f468) > > I remember that we had a discussion on this and BenH had suggested that > we cannot re-audit all the code for correctness with strict-aliasing. So > disable it. That's what -Wstrict-aliasing is for (needs -fstrict-aliasing active as well, to work). Rewriting the code to make it better can be quite a bit of work, of course. But you could e.g. only use -fno-strict-aliasing on those files where -Wstrict-aliasing warns. Segher
Segher Boessenkool <segher@kernel.crashing.org> writes: > On Mon, Aug 08, 2016 at 02:45:35PM +0530, Nikunj A Dadhania wrote: >> >>> In the end, we just might want to add -fno-strict-aliasing to the >> >>> HOSTCFLAGS, just like we already did it in make.rules for the normal >> >>> CFLAGS, and call it a day. >> >> >> >> Or you could fix the problems. SLOF used to work with -fstrict-aliasing, >> >> and it was a nice performance win. >> > >> > That's a question for Nikunj who committed that change a couple of years >> > ago ... >> > (see http://git.qemu.org/?p=SLOF.git;a=commitdiff;h=7eca6a5e56f468) >> >> I remember that we had a discussion on this and BenH had suggested that >> we cannot re-audit all the code for correctness with strict-aliasing. So >> disable it. > > That's what -Wstrict-aliasing is for (needs -fstrict-aliasing active as > well, to work). > > Rewriting the code to make it better can be quite a bit of work, of > course. But you could e.g. only use -fno-strict-aliasing on those files > where -Wstrict-aliasing warns. fill_udp_checksum() was the offending routine. And don't remember there was any warning, we found the hard way. Computed checksum was incorrect in certain cases. Regards Nikunj
diff --git a/tools/sloffs.c b/tools/sloffs.c index 91a23c3..764c956 100644 --- a/tools/sloffs.c +++ b/tools/sloffs.c @@ -405,6 +405,19 @@ sloffs_append(const int file, const char *name, const char *dest) close(out); } +static void print_header_date(void *dptr) +{ + uint8_t *date = dptr; + + if (*(uint64_t *)dptr != 0) { + printf("%02x%02x-%02x-%02x %02x:%02x", date[2], date[3], + date[4], date[5], date[6], date[7]); + } else { + printf("N/A"); + } + +} + static void sloffs_dump(const int fd) { @@ -413,7 +426,6 @@ sloffs_dump(const int fd) struct sloffs file; int i; uint64_t crc; - uint64_t *datetmp; uint64_t header_len; header = sloffs_header(fd); @@ -432,28 +444,10 @@ sloffs_dump(const int fd) /* there is a bug in the date position; * it should be at header->date, but it is at (header->date + 2) */ printf(" Build Date : "); - datetmp = (void *)header->date; - if (be64_to_cpu(*datetmp)) { - printf("%04x", be16_to_cpu(*(uint16_t *)(header->date + 2))); - printf("-%02x", *(uint8_t *)(header->date + 4)); - printf("-%02x", *(uint8_t *)(header->date + 5)); - printf(" %02x:", *(uint8_t *)(header->date + 6)); - printf("%02x", *(uint8_t *)(header->date + 7)); - } else { - printf("N/A"); - } + print_header_date(header->date); printf("\n"); printf(" Modify Date : "); - datetmp = (void *)header->mdate; - if (be64_to_cpu(*datetmp)) { - printf("%04x", be16_to_cpu(*(uint16_t *)(header->mdate + 2))); - printf("-%02x", *(uint8_t *)(header->mdate + 4)); - printf("-%02x", *(uint8_t *)(header->mdate + 5)); - printf(" %02x:", *(uint8_t *)(header->mdate + 6)); - printf("%02x", *(uint8_t *)(header->mdate + 7)); - } else { - printf("N/A"); - } + print_header_date(header->mdate); printf("\n"); printf(" Image Length: %ld", be64_to_cpu(header->flashlen)); printf(" (0x%lx) bytes\n", be64_to_cpu(header->flashlen));
With my version of GCC (v4.8.5 - Advance-Toolchain 7.0) there are currently two warnings when compiling sloffs.c: sloffs.c: In function ‘sloffs_dump’: sloffs.c:437:6: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing] printf("%04x", be16_to_cpu(*(uint16_t *)(header->date + 2))); ^ sloffs.c:449:6: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing] printf("%04x", be16_to_cpu(*(uint16_t *)(header->mdate + 2))); ^ These can be easily fixed by accessing the memory byte by byte instead of casting the pointer to (uint16_t *). And while we're at it, let's also simplify the code a little bit by consolidating the date print code into a separate function which can be used by the two spots that print a date. Signed-off-by: Thomas Huth <thuth@redhat.com> --- tools/sloffs.c | 36 +++++++++++++++--------------------- 1 file changed, 15 insertions(+), 21 deletions(-)