-Werror=... #1220
-Werror=... #1220
Conversation
|
same thing apparently in lib/string/strspn/strrcspn.h. sorry, didn't see that at first |
Thanks! Fixed. |
alejandro-colomar
changed the title
|
@zeha , @hallyn , @ikerexxe , @thesamesam , @jubalh I've enabled |
alejandro-colomar
force-pushed
the
c
branch
4 times, most recently
from
23dbdd2 to
56973ec
Compare
|
This PR is queued after #1205 (that one has some patches that are needed to compile cleanly with -Werror=all). |
|
With gcc-15 I see one warning left on this branch: I guess this might be in #1205 but I haven't checked. |
Yes, it is. Maybe I should just cherry-pick it here and open this PR for review too. |
|
I've cherry-picked the changes from #1205. Let's see if this compiles cleanly everywhere. :) |
This comment was marked as outdated.
This comment was marked as outdated.
|
As packagers, we don't generally want to see forced The conventional way of doing this is a |
|
Hi Sam,
On Fri, Feb 21, 2025 at 09:04:43AM -0800, Sam James wrote:
thesamesam left a comment (shadow-maint/shadow#1220)
As packagers, we [don't generally want to see]
(https://devmanual.gentoo.org/ebuild-writing/common-mistakes/index.html#-werror-compiler-flag-not-removed)
I know.
forced `-Werror`,
It's actually not forced. If you append to CFLAGS, you'll override
prior options, so it's a _default_, not a _forced_ value, IMO.
but because you're only doing it in `autogen.sh` (which we don't use,
and the existence of `autogen.sh` is an antipattern),
Out of ignorance: Why? It's been there since forever, and as you know,
I don't know autotools enough to question it. :)
On the other hand, `-Werror=all` is quite conservative compared to `-Werror`. I guess it would be more or less okay to have `-Werror=all` even in a file that was used by Gentoo.
it doesn't affect us in Gentoo at least.
Ok.
The conventional way of doing this is a `--enable-werror` configure
flag.
Where did that convention grow out of? Did people find it too
inconvenient to do this?
export CFLAGS=-Wno-error
make
What's the benefit of the flag?
Have a lovely night!
Alex
…
|
OK, whatever. It's still considered bad if I have to override it with
It generally means that Sometimes the scripts also do gnarly things like download from the internet as well, so we avoid them.
The issue is the non-determinism based on compiler if
Ah, so there's two issues here:
|
|
Hi Sam,
On Fri, Feb 21, 2025 at 11:36:28AM -0800, Sam James wrote:
thesamesam left a comment (shadow-maint/shadow#1220)
> It's actually not forced. If you append to CFLAGS, you'll override prior options, so it's a _default_, not a _forced_ value, IMO.
OK, whatever. It's still considered bad if I have to override it with
`-Wno-error`.
> On the other hand, `-Werror=all` is quite conservative compared to
> `-Werror`. I guess it would be more or less okay to have
> `-Werror=all` even in a file that was used by Gentoo.
The issue is the non-determinism based on compiler if `-Wall` contents
change. That said, we are okay with specific `-Werror=x` (even if
occasionally, those sometimes cause problems) where `x` is a specific
warning. I don't think I've ever come across someone proposing
`-Werror=all` in a packaging context before though. I agree that it is
better than `-Werror` for distributors. I'm not sure if it's fine for
us though.
Feel free to comment that with other Gentoo maintainers. I'll be
interested if you arrive at a consensus regarding -Werror=all, for my
other projects.
> Where did that convention grow out of? Did people find it too inconvenient to do this?
Ah, so there's two issues here:
1) we usually prefer that if such a flag exists, it's default-off, but
sometimes projects make it default-on based on e.g. presence of
`.git` (or other heuristics to detect a "developer environment"),
and
Sounds consistent with the aversion to -Werror. Makes sense.
2) your example `make` invocation clobbers whatever `CFLAGS` is rather
than appending to it,
That's the standard way to do it today, isn't it?
<https://www.gnu.org/software/make/manual/make.html#Command-Variables>
which is often problematic, so you can't
really do it without inspecting a project's Makefiles first
Heh, this is indeed a historic issue of its own. Historically,
makefiles did work like that, and at some point, someone changed the
behavior so that clobbering CFLAGS would actually append to the real
C flags (which I call now CFLAGS_, out of a lack of imagination, and
wanting to keep it short). Was that autotools invention?
I mean, it makes sense that users usually want to append instead of
overriding, so I like this historical change, but it might have made
more sense to add a new variable to avoid this situation we have now
where some projects use the old semantics of CFLAGS (overriding), and
some use the new semantics of CFLAGS (appending).
I hadn't even realized that the new (appending) semantics existed until
you told me some time ago. (I learned make(1) with very old and simple
examples.)
Have a lovely night!
Alex
…
|
alejandro-colomar
force-pushed
the
c
branch
4 times, most recently
from
1cd434f to
1a48be1
Compare
|
Please don't add Better add some kind of
|
Fixes: d91b22c (2024-07-08; "lib/, src/: Use stpsep() instead of its pattern") Signed-off-by: Alejandro Colomar <[email protected]>
Fixes: 53e1eb4 (2024-07-01; "src/: Remove dead code") Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
Propagate the removal of dead code to its callers, which were only passing the parameter to this function. Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
Closes: <shadow-maint#1218> Reported-by: Chris Hofstaedtler <[email protected]> Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
…ompiled code Signed-off-by: Alejandro Colomar <[email protected]>
I had to omit some really nice ones due to autotools failing badly on them. :/ Signed-off-by: Alejandro Colomar <[email protected]>
Being a macro, the unused return value triggers a diagnostic. This probably needs further investigation, but we have the issue linked below for that. Link: <shadow-maint#1221> Signed-off-by: Alejandro Colomar <[email protected]>
…lly-compiled code Signed-off-by: Alejandro Colomar <[email protected]>
This would be wrong most of the time, but here we're testing corner cases, so let's ignore the diagnostic here. Signed-off-by: Alejandro Colomar <[email protected]>
It's just the obvious thin wrapper around strncpy(3); let's trust it's ok. The reason for removing this test is that GCC has bogus diagnostics for strncpy(3). Its diagnostics are geared towards helping people that abuse strncpy(3) as a poor-man's strlcpy(3) not write exploitable code as easily. Using strncpy(3) for that purpose is brain damaged, and those programs should be audited to stop using this API for that. And most importantly, GCC should stop encouraging writing bad code that calls strncpy(3) as that results in diagnostics that are actively harmful for us, legitimate users of strncpy(3). Those false positives should certainly be out of -Wall. We could fill the tests with pragmas, but let's just remove the tests. I don't feel like maintaining code for ignoring GCC's brain bamage. If people want to write a function for truncating strings (because they can't rely on strlcpy(3) being available, or because they don't like it), they certainly should write such an API. strncpy(3) isn't that API. strncpy(3) is a function that takes a string and writes it into a utmpx(5) member, which is NOT a string. (And I heard it might also be useful for implementing tar(1), which also uses non-terminated character arrays, but I never looked at that code.) Link: <> Signed-off-by: Alejandro Colomar <[email protected]>
This variable is only read if !USE_PAM. Writing to it unconditionally triggers -Werror=unused-but-set-variable errors. Let's wrap it in ifndef to avoid the error. Signed-off-by: Alejandro Colomar <[email protected]>
This avoids a diagnostic from -Wold-style-definition. Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
We were actually triggering undefined behavior to prevent overflow. Do the same thing, but correctly. Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
Signed-off-by: Alejandro Colomar <[email protected]>
…ded VLA Fixes: b3b6d9d (2018-05-15; "Create parent dirs for useradd -m") Cc: Michael Vetter <[email protected]> Cc: Thorsten Kukuk <[email protected]> Signed-off-by: Alejandro Colomar <[email protected]>
The compiler thinks ++depth within the conditional could overflow the type, because it doesn't see that exit(3) will terminate the program. Let's make it easy for the compiler, and only increment the value if we're not going to call exit(3). Signed-off-by: Alejandro Colomar <[email protected]>
Closes: #1218
Reported-by: @zeha
Revisions:
v2
v3
-Werror=all.[[gnu::unused]]. In some cases, we can remove the parameter. In others, we can't but we can unname it. That attribute should only be used when the use of a variable is conditionally compiled.v4
[[gnu::unused]]in parameters used in conditionally-compiled code.v4b
MAYBE_UNUSEDto more parameters.v5
v5b
[[gnu::unused]]to variables used in conditionally-compiled code.v6
v6b
v6c
[[gnu::unused]]with a literal doesn't seem to be valid (see v5). Use a static inline function instead.v7
v7b
v8
v9
v10
-Werror=all. We really needed this thing!!v11
-Wno-error=yaccfor now. We have a PR for removing that code, anyway.v11b
-Wno-error=yaccwas problematic. Let's try-Wno-yacc.v12
v12b
v12c
v12d
v12e
v13
v13b
v13c
-Werror=format-truncation=(false positive).v13d
v14
v14b
v14c
v15
v15b
v16
-Werror=stringop-overread, as it has bogus reports with strncat(3).v17
v18
v19
v19b
v19c
v19d
v19e
v19f
v19g
v19h
v19i
v19j
v19k
v19l
v20
v21
v22
v23
v23b