On 13.03.2016 00:05, Stefan Tauner wrote:
This came up when I was testing if building on SunOS still works on the buildbot's instance of OmniOS r151014 which is based on illumos.
The fix is
- to link against libnsl
- a small C type fix in ich_descriptor_tool
Signed-off-by: Stefan Tauner stefan.tauner@alumni.tuwien.ac.at Acked-by: Stefan Tauner stefan.tauner@alumni.tuwien.ac.at
diff --git a/util/ich_descriptors_tool/ich_descriptors_tool.c b/util/ich_descriptors_tool/ich_descriptors_tool.c index c3d9ba1..ad3b6f0 100644 --- a/util/ich_descriptors_tool/ich_descriptors_tool.c +++ b/util/ich_descriptors_tool/ich_descriptors_tool.c @@ -170,7 +170,7 @@ int main(int argc, char *argv[]) usage(argv, "Seeking to the end of the file failed");
#ifdef HAVE_MMAP
- buf = mmap(NULL, len, PROT_READ, MAP_PRIVATE, fd, 0);
- buf = (uint32_t *)mmap(NULL, len, PROT_READ, MAP_PRIVATE, fd, 0);
The compiler may require that to stop complaining, but AFAICS there is a real bug behind this. buf is sometimes casted to uint8_t *, and unless I'm too sleepy already, this buffer violates the strict aliasing rules. We can solve this with -fno-strict-aliasing, or fix the code. With a code fix, in theory the problem should go away.
if (buf == (void *) -1) #endif {
Side note (no need to fix this right now) util/ich_descriptors_tool/ich_descriptors_tool.c:182 if (len != read(fd, buf, len)) usage(argv, "Seeking to the end of the file failed"); Somehow that error message doesn't fit with the code.
Regards, Carl-Daniel
On Sun, 13 Mar 2016 00:26:41 +0100 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
On 13.03.2016 00:05, Stefan Tauner wrote:
This came up when I was testing if building on SunOS still works on the buildbot's instance of OmniOS r151014 which is based on illumos.
The fix is
- to link against libnsl
- a small C type fix in ich_descriptor_tool
Signed-off-by: Stefan Tauner stefan.tauner@alumni.tuwien.ac.at Acked-by: Stefan Tauner stefan.tauner@alumni.tuwien.ac.at
diff --git a/util/ich_descriptors_tool/ich_descriptors_tool.c b/util/ich_descriptors_tool/ich_descriptors_tool.c index c3d9ba1..ad3b6f0 100644 --- a/util/ich_descriptors_tool/ich_descriptors_tool.c +++ b/util/ich_descriptors_tool/ich_descriptors_tool.c @@ -170,7 +170,7 @@ int main(int argc, char *argv[]) usage(argv, "Seeking to the end of the file failed");
#ifdef HAVE_MMAP
- buf = mmap(NULL, len, PROT_READ, MAP_PRIVATE, fd, 0);
- buf = (uint32_t *)mmap(NULL, len, PROT_READ, MAP_PRIVATE, fd, 0);
The compiler may require that to stop complaining, but AFAICS there is a real bug behind this. buf is sometimes casted to uint8_t *, and unless I'm too sleepy already, this buffer violates the strict aliasing rules. We can solve this with -fno-strict-aliasing, or fix the code. With a code fix, in theory the problem should go away.
While this is true it does not make much sense to try to fix it: - all accesses are read-only. The compiler can re-order as much as it wants, we won't get wrong results. No compiler writers however mean will make this explode. And that's the only potential problem with aliasing. - ich_descriptor.c intentionally uses undefined behavior by writing to some struct members and reading it through others to spare us from bit-shifting/masking. That was discussed at length back when I added it and since we use the code on x86 only it is fine.
if (buf == (void *) -1) #endif {
Side note (no need to fix this right now) util/ich_descriptors_tool/ich_descriptors_tool.c:182 if (len != read(fd, buf, len)) usage(argv, "Seeking to the end of the file failed"); Somehow that error message doesn't fit with the code.
Copy and paste error from :170. Fixed in tested_stuff, thanks!
On 13.03.2016 00:57, Stefan Tauner wrote:
On Sun, 13 Mar 2016 00:26:41 +0100 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
On 13.03.2016 00:05, Stefan Tauner wrote:
This came up when I was testing if building on SunOS still works on the buildbot's instance of OmniOS r151014 which is based on illumos.
The fix is
- to link against libnsl
- a small C type fix in ich_descriptor_tool
Signed-off-by: Stefan Tauner stefan.tauner@alumni.tuwien.ac.at Acked-by: Stefan Tauner stefan.tauner@alumni.tuwien.ac.at
diff --git a/util/ich_descriptors_tool/ich_descriptors_tool.c b/util/ich_descriptors_tool/ich_descriptors_tool.c index c3d9ba1..ad3b6f0 100644 --- a/util/ich_descriptors_tool/ich_descriptors_tool.c +++ b/util/ich_descriptors_tool/ich_descriptors_tool.c @@ -170,7 +170,7 @@ int main(int argc, char *argv[]) usage(argv, "Seeking to the end of the file failed");
#ifdef HAVE_MMAP
- buf = mmap(NULL, len, PROT_READ, MAP_PRIVATE, fd, 0);
- buf = (uint32_t *)mmap(NULL, len, PROT_READ, MAP_PRIVATE, fd, 0);
The compiler may require that to stop complaining, but AFAICS there is a real bug behind this. buf is sometimes casted to uint8_t *, and unless I'm too sleepy already, this buffer violates the strict aliasing rules. We can solve this with -fno-strict-aliasing, or fix the code. With a code fix, in theory the problem should go away.
While this is true it does not make much sense to try to fix it:
- all accesses are read-only. The compiler can re-order as much as it wants, we won't get wrong results. No compiler writers however mean will make this explode. And that's the only potential problem with aliasing.
- ich_descriptor.c intentionally uses undefined behavior by writing to some struct members and reading it through others to spare us from bit-shifting/masking. That was discussed at length back when I added it and since we use the code on x86 only it is fine.
Ah right. Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
if (buf == (void *) -1) #endif {
Side note (no need to fix this right now) util/ich_descriptors_tool/ich_descriptors_tool.c:182 if (len != read(fd, buf, len)) usage(argv, "Seeking to the end of the file failed"); Somehow that error message doesn't fit with the code.
Copy and paste error from :170. Fixed in tested_stuff, thanks!
Thanks.
Regards, Carl-Daniel
On Sun, 13 Mar 2016 08:38:25 +0100 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
On 13.03.2016 00:57, Stefan Tauner wrote:
On Sun, 13 Mar 2016 00:26:41 +0100 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
On 13.03.2016 00:05, Stefan Tauner wrote:
This came up when I was testing if building on SunOS still works on the buildbot's instance of OmniOS r151014 which is based on illumos.
The fix is
- to link against libnsl
- a small C type fix in ich_descriptor_tool
Signed-off-by: Stefan Tauner stefan.tauner@alumni.tuwien.ac.at Acked-by: Stefan Tauner stefan.tauner@alumni.tuwien.ac.at
diff --git a/util/ich_descriptors_tool/ich_descriptors_tool.c b/util/ich_descriptors_tool/ich_descriptors_tool.c index c3d9ba1..ad3b6f0 100644 --- a/util/ich_descriptors_tool/ich_descriptors_tool.c +++ b/util/ich_descriptors_tool/ich_descriptors_tool.c @@ -170,7 +170,7 @@ int main(int argc, char *argv[]) usage(argv, "Seeking to the end of the file failed");
#ifdef HAVE_MMAP
- buf = mmap(NULL, len, PROT_READ, MAP_PRIVATE, fd, 0);
- buf = (uint32_t *)mmap(NULL, len, PROT_READ, MAP_PRIVATE, fd, 0);
The compiler may require that to stop complaining, but AFAICS there is a real bug behind this. buf is sometimes casted to uint8_t *, and unless I'm too sleepy already, this buffer violates the strict aliasing rules. We can solve this with -fno-strict-aliasing, or fix the code. With a code fix, in theory the problem should go away.
While this is true it does not make much sense to try to fix it:
- all accesses are read-only. The compiler can re-order as much as it wants, we won't get wrong results. No compiler writers however mean will make this explode. And that's the only potential problem with aliasing.
- ich_descriptor.c intentionally uses undefined behavior by writing to some struct members and reading it through others to spare us from bit-shifting/masking. That was discussed at length back when I added it and since we use the code on x86 only it is fine.
Ah right. Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Thanks, r1950.
-
Carl-Daniel Hailfinger -
Stefan Tauner