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

messages: fix out of range assertion #11345

Merged
merged 1 commit into from Oct 25, 2016
Merged

Conversation

jcsp
Copy link
Contributor

@jcsp jcsp commented Oct 6, 2016

clang detects that this is invalid (presumably
using an 8 bit type for the enum)

Signed-off-by: John Spray john.spray@redhat.com

@wjwithagen
Copy link
Contributor

@jcsp
Can't fool Clang :)

/usr/srcs/Ceph/work/ceph/src/messages/MMgrReport.h:39:17: warning: comparison of constant 255 with expression of type 'const enum perfcounter_type_d' is always true
[-Wtautological-constant-out-of-range-compare]
assert(type <=255);
~~~~ ^ ~~~

@jcsp
Copy link
Contributor Author

jcsp commented Oct 6, 2016

Gaaah!

@jcsp
Copy link
Contributor Author

jcsp commented Oct 6, 2016

I don't really want to remove the assert, because there's no guarantee the enums are always going to be char-sized :-/

@liewegas
Copy link
Member

liewegas commented Oct 6, 2016

If that still triggers a clang warning we can just change it to 254, which should be just as effective.

@tchaikov
Copy link
Contributor

tchaikov commented Oct 6, 2016

or we can declare the perfcounter_type_d as

enum class perfcounter_type_d : uint8_t {
 // ...
}

@wjwithagen
Copy link
Contributor

@tchaikov @liewegas @jcsp
I did not make it a enum class, but did add the : uint8_t in the declaration.
Clang is now happy.

And I think it also can get ride of a lot of the casts like:
::encode((uint8_t)type, bl)

Have not run it thru GCC

@wjwithagen
Copy link
Contributor

@tchaikov @liewegas @jcsp
Right, removing the casts actually throws Clang into a selction problem on which of the many en/decode options it can promote to to actually get the work done.
So removing the casts is not an option.

@tchaikov
Copy link
Contributor

tchaikov commented Oct 8, 2016

@wjwithagen yeah, less code churn if unscoped version is used, or instead of keeping the casts, we can add

template<typename E,
         typename std::enable_if<std::is_enum<E>{}>::type* = nullptr>
void encode(E e, bufferlist& bl) {
  encode(static_cast<typename std::underlying_type<E>::type>(e), bl);
}

to delegate the encode() calls applying to the enum types to their underlying types.

@jcsp
Copy link
Contributor Author

jcsp commented Oct 17, 2016

Updated this to always use uint8_t for the enum, and just assert on the size of the resulting type (the assertion is still strictly always true, so let's see if clang complains about that too!)

@@ -36,7 +36,7 @@ class PerfCounterType
::encode(path, bl);
::encode(description, bl);
::encode(nick, bl);
assert(type < 256);
assert(sizeof(type) == 1); // enum type declared as uint8_t
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, could be static_assert().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could be

static_assert (sizeof(type) == 1 , "enum type declared as uint8_t");
static_assert ( bool_constexpr )    //  (since C++17)

When clang uses an 8 bit type for the enum, it
complains (out of range) if comparing <256,
and complains (tautological) if comparing <=256.

Avoid this by explicitly making the enum an
uint8_t, and just asserting that that it has
that size at the point that we assume so for
the encoding (in case someone modified the
type definition without checking how it was used).

Signed-off-by: John Spray <john.spray@redhat.com>
@tchaikov
Copy link
Contributor

lgtm.

@tchaikov tchaikov self-assigned this Oct 25, 2016
@tchaikov tchaikov merged commit 0919de2 into ceph:master Oct 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants