Remove some maybe-uninitialized warnings #5516
Conversation
In the Eigen matrix type_caster, resize the matrix instead of assigning with a new one when the matrix size needs to be adjusted. This can remove lots of compiling warnings about "maybe-uninitialized".
|
Could you please report how exactly you got the maybe-uninitialized warnings (compiler details, options, error messages)? I think the change is fine, although I want to ask someone familiar with Eigen and numpy to take a look, but let's get the background info complete first. |
|
Hi, @rwgk, here is a simple example to reproduce the warning.
cmake_minimum_required(VERSION 3.30)
project(test_pybind
LANGUAGES CXX
)
set(CMAKE_VERBOSE_MAKEFILE ON)
set(CMAKE_CXX_STANDARD 17)
set(CMAKE_CXX_STANDARD_REQUIRED ON)
set(CMAKE_CXX_EXTENSIONS OFF)
set(CMAKE_CXX_FLAGS "-Wall -Wextra")
set(CMAKE_CXX_FLAGS_RELEASE "-O3 -DNDEBUG")
find_package(Eigen3 REQUIRED)
find_package(pybind11 REQUIRED)
pybind11_add_module(binding binding.cpp)
target_link_libraries(binding
PRIVATE Eigen3::Eigen
)EDIT: The include order below is incorrect, please see Lines 81 to 86 in d2e7e8c
#include <Eigen/Dense>
#include <pybind11/eigen.h>
#include <pybind11/numpy.h>
#include <pybind11/pybind11.h>
// good
Eigen::MatrixXd add_matrix(const Eigen::Ref<const Eigen::MatrixXd> &m1,
const Eigen::Ref<const Eigen::MatrixXd> &m2) {
return m1 + m2;
}
// good
Eigen::MatrixXd add_matrix2(const Eigen::MatrixXd &m1,
const Eigen::MatrixXd &m2) {
return m1 + m2;
}
// this one causes "maybe-uninitialized" warning
Eigen::Vector4d add_vector(const Eigen::Vector4d &v1,
const Eigen::Vector4d &v2) {
return v1 + v2;
}
PYBIND11_MODULE(binding, m) {
namespace py = pybind11;
m.def("add_matrix", &add_matrix, "Add two matrices", py::arg("m1"),
py::arg("m2"))
.def("add_matrix2", &add_matrix2, "Add two matrices", py::arg("m1"),
py::arg("m2"))
.def("add_vector", &add_vector, "Add two vectors", py::arg("v1"),
py::arg("v2"));
} |
|
That's great but I was simply looking for the compiler info (e.g. gcc --version), one example compilation command (e.g. g++ ...), and the error message. It's usually/often very troublesome to reproduce something based on guessing such details. |
|
I tested both GCC13: 13.3.1 |
|
Sharing this here: Is this a common problem when using Eigen::matrix (unrelated to pybind11)? /usr/bin/g++-13 -fPIC -Wall -Wextra -O3 -DNDEBUG -flto -shared -o binding.cpython-313-x86_64-linux-gnu.so CMakeFiles/binding.dir/binding.cpp.o Key Observations: This is a warning, not an error, meaning the compiler detected a possible but not guaranteed uninitialized usage. The warning appears when loading an Eigen matrix through Pybind11, specifically at: value might be default-constructed before assignment, which could trigger the warning.
cpp cpp
cpp
|
|
Also: What do you think about this solution? diff --git a/include/pybind11/eigen/matrix.h b/include/pybind11/eigen/matrix.h
ChatGPT said: cpp cpp Why This Fix Works Type(fits.rows, fits.cols); constructs a new matrix but might leave memory uninitialized, depending on the constructor behavior. Eigen::Matrix::resize() ensures the matrix is properly resized, potentially avoiding unnecessary allocations and keeping existing memory where possible. Eigen::Matrix::resize() is the preferred way to modify an existing matrix's shape instead of reassigning. cpp cpp Verdict: Good Fix! |
|
This ChatGPT "Any Downsides" was on my mind:
This would only be a problem if I'm OK accepting it. @seberg do you have an opinion? |
|
Thanks for the ping. Unfortunately, I am not familiar with the code (and especially how a user would use it!). Neither it is really clear to me why the compiler doesn't like things. Now, is this safe? As you said, mutating then this would be clearly wrong. |
|
I don't think it will be wrong. The changed line manages memory allocation in a way that the compiler prefers. At line 328, data from the numpy array is copied into |
|
Could you please try a super quick experiment? Move the Eigen include after the pybind11 includes like this: Do the warnings go away? |
|
I spent >20 minutes trying to reproduce the warning but I wasn't successful. (I only got a successful build without warnings, using gcc 13.3.0.) Could you please post complete information here?
(The command you posted before is for linking, not compiling.) |
|
@rwgk Thank you for trying to fix this issue. Here is the output I got: With GNU 13.3.1With GNU 14.2.1:I also tried the include order you suggested. I got the same output shown as above. |
|
I notice that link time optimization is enabled. I turned it off and tried again with 13.3.1 |
The important question (and maybe the answer is "obviously nothing"), is if anything might have happened with the existing |
This reverts commit 7d5a9b4.
|
Here is a minimal reproducer (based on these outputs): #include <pybind11/pybind11.h>
#include <pybind11/eigen.h>
#include <Eigen/Dense>
void pass_vector(const Eigen::Vector4d &) {}
PYBIND11_MODULE(binding, m) {
m.def("pass_vector", &pass_vector);
}g++ -Ipybind11/include -isystem /usr/include/python3.12 -isystem /usr/include/eigen3 -Wall -Wextra -O3 -DNDEBUG -std=c++17 -fPIC -fvisibility=hidden -c pr5516_reproducer.cppWith master @ d2e7e8c: |
|
@daizhirui JIC we want to make more changes: Please do not force push to this branch, it will break the references to commits. I added two commits: Rationale:
I tested the warning suppression with the command shown under the comment above. @daizhirui Could you please confirm that the warning suppression solves your original problem? |
|
I forgot to add: Ideally we'd add a test in GitHub Actions that fails before this PR, but I'm out of time. Help appreciated. I'm OK though merging this PR without a test, after @daizhirui confirms that it solves the original problem. |
|
BTW, please see: Lines 81 to 86 in d2e7e8c It doesn't make a difference for the reproducer, but good to keep in mind at all times. |
|
@rwgk Thanks for reminding the header order. I fixed it when I ran the reproducer. |
Ugh. Could you please provide a minimal reproducer similar to what I posted above? |
|
I tried the reproducer you proposed here #5516 (comment) I read the Eigen docs about assignment and resizing: link. I think the modification I propose keeps the same behavior and is more efficient. As mentioned in Eigen docs, operator= causes resizing |
Huh, why? (And what did you observe? Please be more thorough in providing ground truth information.) Please reduce your problem. Start with your original code, then strip it down as much as possible. In your case I expect that'll take only a few minutes.
It's a behavior change that could send others down a rabbit hole. I've seen situations where people spent days to debug what subtle change (like you're original one) broke their service. |
|
I added lines as you proposed: PYBIND11_WARNING_PUSH
PYBIND11_WARNING_DISABLE_GCC("-Wmaybe-uninitialized") // See PR #5516
// Allocate the new type, then build a numpy reference into it
value = Type(fits.rows, fits.cols);
PYBIND11_WARNING_POPAnd with the same mini reproducer: #include <pybind11/pybind11.h>
#include <pybind11/eigen.h>
#include <Eigen/Dense>
void pass_vector(const Eigen::Vector4d &) {}
PYBIND11_MODULE(binding, m) {
m.def("pass_vector", &pass_vector);
}and the following CMakeLists.txt: cmake_minimum_required(VERSION 3.30)
project(test_pybind
LANGUAGES CXX
)
set(CMAKE_VERBOSE_MAKEFILE ON)
set(CMAKE_CXX_STANDARD 17)
set(CMAKE_CXX_STANDARD_REQUIRED ON)
set(CMAKE_CXX_EXTENSIONS OFF)
set(CMAKE_CXX_FLAGS "-Wall -Wextra")
set(CMAKE_CXX_FLAGS_RELEASE "-O3 -DNDEBUG")
add_subdirectory(pybind11)
find_package(Eigen3 REQUIRED)
pybind11_add_module(binding binding.cpp)
target_link_libraries(binding
PRIVATE Eigen3::Eigen
), I get the following output: When I commented
|
|
I'll merge this PR, it clearly helps at least a little bit, based on the reproducer I posted. |
Description
Add a GCC
-Wmaybe-uninitializedwarning suppression in pybind11/eigen/matrix.hSee #5516 (comment) for a situation in which this helps.
Suggested changelog entry:
A ``-Wmaybe-uninitialized`` warning suppression was added in pybind11/eigen/matrix.h