Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Build powerpc64 with -Wno-stringop-overflow tests due to false positives #178

Closed
wants to merge 1 commit into from

Conversation

avsm
Copy link
Member

@avsm avsm commented Jun 4, 2023

They do seem to be false positives when I look through the code, and it is also independently reported here:
https://bugs.launchpad.net/ubuntu/+source/gcc-12/+bug/2013140

The warnings disappear at -O2, but compilation and tests seem fine at -O3 with the warning disabled, so I chose to keep it at -O3.

An example warning is:

In file included from native/poly1305-donna.c:11:
native/poly1305-donna.c: In function ‘mc_poly1305_update’:
native/poly1305-donna-64.h:24:23: note: at offset 28 into destination object ‘buffer’ of size 16
   24 |         unsigned char buffer[poly1305_block_size];
      |                       ^~~~~~
In function ‘poly1305_update’,
    inlined from ‘mc_poly1305_update’ at native/poly1305-donna.c:60:3:
native/poly1305-donna.c:48:54: error: writing 1 byte into a region of size 0 [-Werror=stringop-overflow=]
   48 |                         st->buffer[st->leftover + i] = m[i];
      |                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~

Spotted while testing ocaml/ocaml#12276

They do seem to be false positives when I look through the code, and
it is also independently reported here:
https://bugs.launchpad.net/ubuntu/+source/gcc-12/+bug/2013140

The warnings disappear at -O2, but compilation and tests seem fine at
-O3 with the warning disabled, so I chose to keep it at -O3.

An example warning is:

```
In file included from native/poly1305-donna.c:11:
native/poly1305-donna.c: In function ‘mc_poly1305_update’:
native/poly1305-donna-64.h:24:23: note: at offset 28 into destination object ‘buffer’ of size 16
   24 |         unsigned char buffer[poly1305_block_size];
      |                       ^~~~~~
In function ‘poly1305_update’,
    inlined from ‘mc_poly1305_update’ at native/poly1305-donna.c:60:3:
native/poly1305-donna.c:48:54: error: writing 1 byte into a region of size 0 [-Werror=stringop-overflow=]
   48 |                         st->buffer[st->leftover + i] = m[i];
      |                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~
```
@hannesm
Copy link
Member

hannesm commented Jun 4, 2023

I'm uneasy with this change. It is a big hammer for such an issue: now we compile everything on power without -Wno-stringop-overflow -- is this the intention? Could we disable the warning more locally? Could we check for the specific compiler version(s) that encounter this "false positive" (is it confirmed by the gcc folks that it is a false positive?)

FWIW, why is it only added to opt_flags?

Isn't that just a compiler warning, and -Werror is only enabled in dev profile (via src/dune)? Shouldn't that the place be to express -Werror, but on power do not enable no-stringop-overflow as error (but leave as warning)?

@avsm
Copy link
Member Author

avsm commented Jun 4, 2023

It's not confirmed by upstream (as far as I can tell, but I haven't finished trawling the gcc bug tracker yet). I added it to opt_flags since it only strikes when -O3 is active, but changing this to -Wno-error-stringop-overflow would avoid disabling it and also avoid Werroring (but make the build quite spammy). Perhaps thats better though -- if there is a compiler bug, it should at least be visible.

@hannesm
Copy link
Member

hannesm commented Jul 31, 2023

So indeed, "if there's a compiler bug, it should at least be visible". Do you plan to work on taking a slightly different road (disable the flags only in the specific environment where they're needed to be excluded)?

What about the CI, is there any for powerpc64 that fails? If not, could this be added?

@hannesm
Copy link
Member

hannesm commented Sep 18, 2023

thanks for your work, again. I merged #184, where the no-strinop-overflow is applied in a much smaller context.

@hannesm hannesm closed this Sep 18, 2023
hannesm added a commit to hannesm/opam-repository that referenced this pull request Sep 18, 2023
…age, mirage-crypto-rng-lwt, mirage-crypto-rng-eio, mirage-crypto-rng-async, mirage-crypto-pk and mirage-crypto-ec (0.11.2)

CHANGES:

* mirage-crypto-rng-eio: improve portability by using eio 0.7's monotonic clock
  interface instead of mtime.clock.os. (mirage/mirage-crypto#176 @TheLortex)
* mirage-crypto-rng-eio: update to eio 0.12 (mirage/mirage-crypto#182 @talex5)
* mirage-crypto-rng: fix typo in RNG setup (mirage/mirage-crypto#179 @samueldurantes)
* macOS: on arm64 with clang 14.0.3, avoid instcombine (due to miscompilations)
  reported by @samoht mit-plv/fiat-crypto#1606 (comment)
  re-reported in ulrikstrid/ocaml-jose#63 and mirleft/ocaml-tls#478
  (mirage/mirage-crypto#185 @hannesm @kit-ty-kate)
* avoid "stringop-overflow" warning on PPC64 and S390x (spurious warnings) when
  in devel mode (mirage/mirage-crypto#178 mirage/mirage-crypto#184 @avsm @hannesm)
* stricter C prototypes, unsigned/signed integers (mirage/mirage-crypto#175 @MisterDA @haesbaert
  @avsm @hannesm)
* support DragonFlyBSD (mirage/mirage-crypto#181 @movepointsolutions)
* support GNU/Hurd (mirage/mirage-crypto#174 @pinotree)
nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
…age, mirage-crypto-rng-lwt, mirage-crypto-rng-eio, mirage-crypto-rng-async, mirage-crypto-pk and mirage-crypto-ec (0.11.2)

CHANGES:

* mirage-crypto-rng-eio: improve portability by using eio 0.7's monotonic clock
  interface instead of mtime.clock.os. (mirage/mirage-crypto#176 @TheLortex)
* mirage-crypto-rng-eio: update to eio 0.12 (mirage/mirage-crypto#182 @talex5)
* mirage-crypto-rng: fix typo in RNG setup (mirage/mirage-crypto#179 @samueldurantes)
* macOS: on arm64 with clang 14.0.3, avoid instcombine (due to miscompilations)
  reported by @samoht mit-plv/fiat-crypto#1606 (comment)
  re-reported in ulrikstrid/ocaml-jose#63 and mirleft/ocaml-tls#478
  (mirage/mirage-crypto#185 @hannesm @kit-ty-kate)
* avoid "stringop-overflow" warning on PPC64 and S390x (spurious warnings) when
  in devel mode (mirage/mirage-crypto#178 mirage/mirage-crypto#184 @avsm @hannesm)
* stricter C prototypes, unsigned/signed integers (mirage/mirage-crypto#175 @MisterDA @haesbaert
  @avsm @hannesm)
* support DragonFlyBSD (mirage/mirage-crypto#181 @movepointsolutions)
* support GNU/Hurd (mirage/mirage-crypto#174 @pinotree)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants