-
Notifications
You must be signed in to change notification settings - Fork 6k
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
cls/rbd: Silence gcc7 maybe-uninitialized warning #18504
cls/rbd: Silence gcc7 maybe-uninitialized warning #18504
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good finding. But would it be a workaround for the existing gcc bug, still ?
@joscollin if you are referring to https://bugzilla.redhat.com/show_bug.cgi?id=1439647 it was fixed in gcc-7.1.1-3.fc26. I'm using the current version, gcc-7.2.1-2.fc26.x86_64. |
We shouldn't be forced to create copy constructors when C++ should automatically provide a default. If we cannot trust that a std::string is properly copied, I think there are bigger issues w/ the compiler. That |
It's actually the int64_t that appears to be the problem. It appears to have something to do with the copies boost::variant is making and the copy constructor was just a guess that happened to resolve the warning. Not sure how much time we want to throw at this but I guess I can look deeper into it. |
6ad235c
to
5df732f
Compare
@dillaman I've created a bug for this issue at https://bugzilla.redhat.com/show_bug.cgi?id=1507358 but it's not at all clear how long that will take to resolve. I've pushed an alternative patch which changes the order of the member variables and, I hope, should be lower impact given I don't think this is a public API? I know we shouldn't have to jump through these sorts of hoops to work around compiler bugs but, if this is a low impact change, maybe we can slip this in with the snapshot_id initialisation change? |
@badone Why not limit the scope of the changes to the single file? diff --git a/src/cls/rbd/cls_rbd_types.h b/src/cls/rbd/cls_rbd_types.h
index b82bf452e5..98c5904a57 100644
--- a/src/cls/rbd/cls_rbd_types.h
+++ b/src/cls/rbd/cls_rbd_types.h
@@ -252,13 +252,13 @@ struct GroupSnapshotNamespace {
GroupSnapshotNamespace(int64_t _group_pool,
const string &_group_id,
- const snapid_t &_snapshot_id) :group_pool(_group_pool),
- group_id(_group_id),
- snapshot_id(_snapshot_id) {}
+ snapid_t _snapshot_id)
+ : group_id(_group_id), group_pool(_group_pool), snapshot_id(_snapshot_id) {
+ }
+ std::string group_id;
int64_t group_pool = 0;
- string group_id;
- snapid_t snapshot_id;
+ snapid_t snapshot_id = CEPH_NOSNAP;
void encode(bufferlist& bl) const;
void decode(bufferlist::iterator& it);
|
gcc7 warns that GroupSnapshotNamespace::group_pool may be used uninitialized in SnapshotNamespaceOnDisk::decode. Reordering the member variables silences the spurious warning. Signed-off-by: Brad Hubbard <bhubbard@redhat.com>
5df732f
to
772880f
Compare
@dillaman Sure, that's fine, done. |
submodules for project are unmodified |
all commits in this PR are signed |
OK - docs built |
make check succeeded |
1 similar comment
make check succeeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
gcc7 warns that GroupSnapshotNamespace::group_pool may be use
uninitialized in SnapshotNamespaceOnDisk::decode. Adding a non-default
copy constructor resolves this.
Signed-off-by: Brad Hubbard bhubbard@redhat.com