Open jwnimmer-tri opened 7 months ago
So... the -Wuninitialized
look "legit", but I'm not sure if we want to enable them or not. The ones I've looked at so far are the compiler being unhappy about copying uninitialized Eigen types.
-Warray-bounds
and -Wstringop-overflow
trip in the bowels of fmt
in a way that it is not clear what Drake code is even causing them. The compiler makes them seem legitimate, but I can't see any way to address them from the Drake side, if indeed it is even possible to do so.
At least one instance of -Wno-stringop-overread
looks legitimate, but is in the bowels of Eigen. Specifically, it looks like Eigen's assignment operators use vector optimizations built around 16-byte groups, which can be invoked on a Vector1d
, which of course has size 8. This would presumably need to be addressed upstream.
There are many instances of -Wdangling-reference
that "look legitimate". In short, the compiler doesn't like methods that return references that are members of temporary objects, even if those members are themselves references initialized from other references that outlive the temporaries. (IMHO this whole pattern is dodgy and probably ought to use some sort of non-nullable pointer wrapper instead, but it is pervasive and not limited to just Drake itself; common_robotics_utilities
also contributes.)
-Wuninitialized
seems to trip a lot when a partly-initialized type (often an Eigen type) gets copied. These are technically legitimate, but most are likely not issues in practice, and trying to squash them without explicitly zeroing (or otherwise setting) values, likely at a performance cost, doesn't appear practical. I did not investigate -Wmaybe-uninitialized
as those are likely more of the same. (I expect that -Wmaybe-uninitialized
is a superset of -Wuninitialized
.)
-Wpessimizing-move
appears legitimate and should be fixed and enabled for production. On the plus side, it appears there is only one instance thereof.
-Wpessimizing-move
. The rest need deeper investigation and may need to be left disabled (see previous comment).To help circulate the discussion among a larger audience of Drake developers, I would like to have links here to sample compiler output with the errors (with source line numbers etc.).
Possibly the easiest way to do that would be to open Draft PRs where we switch the warnings to errors (and so, the Noble CI would fail)? That could be 6 PRs with one -Werror=...
promotion each, or if it makes sense to group e.g. -Werror=uninitialized
and -Werror=maybe-uninitialized
into the same PR, that's fine too.
If you have other ideas for how to show examples, that's fine too.
Okay, since there is some conflation between -Warray-bounds
, -Wstringop-overflow
and -Wno-stringop-overread
, I turned all on at the same time. Logs are here.
Similarly, combined logs for -Wuninitialized
and -Wmaybe-uninitialized
are here.
Finally, logs from -Wdangling-reference
are here.
All of the above were created on an ubuntu:noble
Docker container using ef875d7a5dc8 (with obvious modifications to tools/skylark/drake_cc.bzl
).
Ugh, I was hoping that dangling-reference
would be one we could keep but those false positives are just too awful. We'll need to turn that one off permanently.
For the rest, I'll look at the rest & seek other reviewer's inputs a bit later on.
It's OK if you want to wait until we have the votes on all of the decisions before you proceed with any changes.
Towards #21335.
Ubuntu 24.04 uses GCC 13 by default. GCC 13 triggers some new warnings in Drake, with apparently a mix of true positives and false positives. We'll need to investigate and resolve things one way or another.
Known problems so far:
-Warray-bounds
-Wdangling-reference
-Wmaybe-uninitialized
-Wpessimizing-move
-Wstringop-overflow
-Wstringop-overread
-Wuninitialized
Here is the place where we previously nerfed them in a prior PR. The goal here is to revisit them one by one: https://github.com/RobotLocomotion/drake/pull/21338/files#diff-03065ba30d68e2a56c9cf5059da65b79d9ae54c70f0e45aa76912a6e7d705b00
For each one, decide whether:
(1) The warnings are primarily true positives. In this case, we want to repair our code and then promote the warning back to an error.
(2) The warnings are primarily false positives. In this case, should keep it globally disabled, and put a little comment nearby explaining our rationale.