Bug 61414 - enum class bitfield size-checking needs a separate warning flag controlling it
Summary: enum class bitfield size-checking needs a separate warning flag controlling it
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: unknown
: P3 normal
Target Milestone: 8.4
Assignee: Jakub Jelinek
URL: https://gcc.gnu.org/ml/gcc-patches/20...
Keywords: diagnostic, patch
: 92182 (view as bug list)
Depends on:
Blocks: 44209
  Show dependency treegraph
 
Reported: 2014-06-04 17:30 UTC by Tom Tromey
Modified: 2020-02-16 17:49 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2016-09-28 00:00:00


Attachments
gcc10-pr61414.patch (1.90 KB, patch)
2019-11-25 15:56 UTC, Jakub Jelinek
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tom Tromey 2014-06-04 17:30:28 UTC
Consider this source:

enum class K : unsigned int {
  V = 27
};

struct S {
  K v : 5;
};

When I compile this with Fedora 20 g++ (4.8.2-based) I get:

barimba. g++ --syntax-only -pedantic -std=c++11 /tmp/q.cc
/tmp/q.cc:6:9: warning: ‘S::v’ is too small to hold all values of ‘enum class K’ [enabled by default]
   K v : 5;
         ^

However, 5 bits is sufficient to hold all the values of K.
The warning goes away if I increase the size to 32, though of
course that defeats the purpose.
Comment 1 Tom Tromey 2014-06-04 17:44:06 UTC
Jonathan pointed out that this is not really a bug because
an enumeration with a fixed underlying type has a different
definition of its underlying values.

However, the bug still exists if the underlying type is not fixed:

enum class K {
  V = 27
};

struct S {
  K v : 6;
};

barimba. g++ --syntax-only -pedantic -std=c++11 /tmp/q.cc
/tmp/q.cc:6:9: warning: ‘S::v’ is too small to hold all values of ‘enum class K’ [enabled by default]
   K v : 6;
         ^
Comment 2 Daniel Krügler 2014-06-04 18:42:48 UTC
(In reply to Tom Tromey from comment #1)
> However, the bug still exists if the underlying type is not fixed:
> 
> enum class K {
>   V = 27
> };

This enum K also has a fixed underlying type (int). Contrary to unscoped enums, *all* scoped enums have a pre-defined underlying type that does not depend on the individual enumerators.
Comment 3 Tom Tromey 2014-06-04 18:46:07 UTC
(In reply to Daniel Krügler from comment #2)
> (In reply to Tom Tromey from comment #1)
> > However, the bug still exists if the underlying type is not fixed:
> > 
> > enum class K {
> >   V = 27
> > };
> 
> This enum K also has a fixed underlying type (int). Contrary to unscoped
> enums, *all* scoped enums have a pre-defined underlying type that does not
> depend on the individual enumerators.

Hah, shows what I know.

Regardless, this warning does not seem to be useful to me.
Comment 4 Pavel Revak 2014-12-11 21:27:19 UTC
I think that this warning is not on right place.

for example if I use bitfield:

struct S {
  unsigned int v : 5;
};

then this works without any warn,
so why I can't use enum class typed to unsigned int?
Comment 5 Xidorn Quan 2016-03-10 03:38:27 UTC
It seems G++ always throw that warning for enum class as bitfield, even when the enum class is actually empty:
> enum class K {};
> struct S {
>   K v : 5;
> };

G++ would give:
> test.cpp:3:9: warning: 'S::v' is too small to hold all values of 'enum class K'
>    K v : 5;
Comment 6 Jonathan Wakely 2016-09-28 23:04:28 UTC
(In reply to Xidorn Quan from comment #5)
> It seems G++ always throw that warning for enum class as bitfield, even when
> the enum class is actually empty:
> > enum class K {};
> > struct S {
> >   K v : 5;
> > };
> 
> G++ would give:
> > test.cpp:3:9: warning: 'S::v' is too small to hold all values of 'enum class K'
> >    K v : 5;

Yes, read comment 2.
Comment 7 Jonathan Wakely 2016-09-28 23:06:00 UTC
PR 51242 discusses the same issue, and PR 51242 comment 27 notes that Clang only warns when a value that isn't known to fit is assigned to the bitfield, which seems more useful.

At the very least there should be a -Wxxx switch controlling this option so it can be disabled.
Comment 8 Matt Godbolt 2016-10-27 19:34:19 UTC
Definitely a plus one to a switch to disable the warning: we currently have to work around this by putting code that does this in an -Isystem directory!
Comment 9 eric-bugs 2017-06-28 17:52:35 UTC
This does not seem like correct behavior to me either. The warning should be based on the maximum declared enum value, not the maximum possible value held by the underlying type.

After all as of C++17, the standard makes it undefined what happens if you try to stuff an integer into an enum value that doesn't correspond to one of the values listed in the enum declaration.
Comment 10 Tom Tromey 2017-07-24 13:26:29 UTC
I ran into this again, went to file a bug, and then found that
I'd already filed the bug...
Comment 11 Erik Rainey 2017-09-09 21:25:48 UTC
Present in 5.4 through 7.2 at least.
Comment 12 Jonathan Wakely 2017-10-16 00:05:05 UTC
(In reply to eric-bugs from comment #9)
> This does not seem like correct behavior to me either. The warning should be
> based on the maximum declared enum value, not the maximum possible value
> held by the underlying type.

No.

> After all as of C++17, the standard makes it undefined what happens if you
> try to stuff an integer into an enum value that doesn't correspond to one of
> the values listed in the enum declaration.

What? No it doesn't.
Comment 13 Sam van Kampen 2017-10-16 12:39:05 UTC
I've sent a patch that adds the warning flag -Wbitfield-enum-conversion to gcc-patches, and the gcc mailing list now also has a thread on the bug, and I thought I might share them on the bug tracker:

The patch:
https://gcc.gnu.org/ml/gcc-patches/2017-10/msg00989.html

The thread on the bug:
https://gcc.gnu.org/ml/gcc/2017-10/msg00121.html
Comment 14 Eric Gallager 2017-10-16 15:01:27 UTC
(In reply to Jonathan Wakely from comment #7)
> At the very least there should be a -Wxxx switch controlling this option so
> it can be disabled.

Agreed, that falls under bug 44209; retitling this one accordingly.

(In reply to Sam van Kampen from comment #13)
> I've sent a patch that adds the warning flag -Wbitfield-enum-conversion to
> gcc-patches, and the gcc mailing list now also has a thread on the bug, and
> I thought I might share them on the bug tracker:
> 
> The patch:
> https://gcc.gnu.org/ml/gcc-patches/2017-10/msg00989.html
> 
> The thread on the bug:
> https://gcc.gnu.org/ml/gcc/2017-10/msg00121.html

The name chosen for the flag makes it sound kinda related to bug 39170 but I guess the "enum" in the middle is enough of a differentiator.
Comment 15 Xavier B 2018-10-18 09:48:39 UTC
hi,

Just a ping for this issue.

Since it's about declarations and it is not possible to disable it,
as soon as you have headers files using the feature you get flooded with thousands of warnings making it impossible to see the other actually useful warnings...
Comment 16 Jonathan Wakely 2018-10-18 12:08:24 UTC
Sam, did you get a chance to implement the changes requested on the mailing list?
Comment 17 Barry Revzin 2019-04-25 21:27:20 UTC
Hi, it's me, another programmer who would love to use enum classes in bitfields instead of having to static_cast back and forth :-) Can anybody fixup Sam's patch?
Comment 18 Eric Gallager 2019-10-29 17:44:43 UTC
*** Bug 92182 has been marked as a duplicate of this bug. ***
Comment 19 Dávid Bolvanský 2019-11-04 11:22:10 UTC
5 years...
Can anybody fix it? It is real issue on real world code:


https://reviews.llvm.org/D69792
Comment 20 Alexander Kokushkin 2019-11-23 19:55:35 UTC
Unfortunately, it's still present in 9.2.0 and quite painful.
Comment 21 Jakub Jelinek 2019-11-25 15:56:49 UTC
Created attachment 47352 [details]
gcc10-pr61414.patch

Untested fix.
Comment 22 Eric Gallager 2019-11-26 06:33:46 UTC
Patch discussion on the gcc-patches mailing list starts here for this month: https://gcc.gnu.org/ml/gcc-patches/2019-11/msg02329.html
Comment 23 Jakub Jelinek 2019-11-26 21:57:58 UTC
Author: jakub
Date: Tue Nov 26 21:57:27 2019
New Revision: 278736

URL: https://gcc.gnu.org/viewcvs?rev=278736&root=gcc&view=rev
Log:
	PR c++/61414
	* c-attribs.c (handle_mode_attribute): Add mode attribute to
	ENUMERAL_TYPEs.

	* class.c (enum_to_min_precision): New hash_map.
	(enum_min_precision): New function.
	(check_bitfield_decl): Use it.

	* g++.dg/cpp0x/enum23.C: Remove xfail.
	* g++.dg/cpp0x/enum28.C: New test.

Added:
    trunk/gcc/testsuite/g++.dg/cpp0x/enum38.C
Modified:
    trunk/gcc/c-family/ChangeLog
    trunk/gcc/c-family/c-attribs.c
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/class.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/g++.dg/cpp0x/enum23.C
Comment 24 Jakub Jelinek 2019-12-20 17:00:42 UTC
Author: jakub
Date: Fri Dec 20 17:00:02 2019
New Revision: 279654

URL: https://gcc.gnu.org/viewcvs?rev=279654&root=gcc&view=rev
Log:
	Backported from mainline
	2019-11-26  Jakub Jelinek  <jakub@redhat.com>

	PR c++/61414
	* c-attribs.c (handle_mode_attribute): Add mode attribute to
	ENUMERAL_TYPEs.

	* class.c (enum_to_min_precision): New hash_map.
	(enum_min_precision): New function.
	(check_bitfield_decl): Use it.

	* g++.dg/cpp0x/enum23.C: Remove xfail.
	* g++.dg/cpp0x/enum28.C: New test.

Added:
    branches/gcc-9-branch/gcc/testsuite/g++.dg/cpp0x/enum38.C
Modified:
    branches/gcc-9-branch/gcc/c-family/ChangeLog
    branches/gcc-9-branch/gcc/c-family/c-attribs.c
    branches/gcc-9-branch/gcc/cp/ChangeLog
    branches/gcc-9-branch/gcc/cp/class.c
    branches/gcc-9-branch/gcc/testsuite/ChangeLog
    branches/gcc-9-branch/gcc/testsuite/g++.dg/cpp0x/enum23.C
Comment 25 CVS Commits 2020-02-14 16:34:30 UTC
The releases/gcc-8 branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:22a7fa8517063c76b069d2b08dca5a9d270798f8

commit r8-9985-g22a7fa8517063c76b069d2b08dca5a9d270798f8
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Fri Feb 14 12:45:30 2020 +0100

    backport: re PR c++/61414 (enum class bitfield size-checking needs a separate warning flag controlling it)
    
    	Backported from mainline
    	2019-11-26  Jakub Jelinek  <jakub@redhat.com>
    
    	PR c++/61414
    	* c-attribs.c (handle_mode_attribute): Add mode attribute to
    	ENUMERAL_TYPEs.
    
    	* class.c (enum_to_min_precision): New hash_map.
    	(enum_min_precision): New function.
    	(check_bitfield_decl): Use it.
    
    	* g++.dg/cpp0x/enum23.C: Remove xfail.
    	* g++.dg/cpp0x/enum28.C: New test.
Comment 26 CVS Commits 2020-02-14 16:41:02 UTC
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:519a33f954fd71cb8b74053e168e23a1cb00d30b

commit r10-6637-g519a33f954fd71cb8b74053e168e23a1cb00d30b
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Fri Feb 14 17:36:00 2020 +0100

    c++: Fix thinko in enum_min_precision [PR61414]
    
    When backporting the PR61414 fix to 8.4, I've noticed that the caching
    of prec is actually broken, as it would fail to actually store the computed
    precision into the hash_map's value and so next time we'd think the enum needs
    0 bits.
    
    2020-02-14  Jakub Jelinek  <jakub@redhat.com>
    
    	PR c++/61414
    	* class.c (enum_min_precision): Change prec type from int to int &.
    
    	* g++.dg/cpp0x/enum39.C: New test.
Comment 27 CVS Commits 2020-02-14 16:42:59 UTC
The releases/gcc-9 branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:f0a72494ea3747d1f66a0cda3e67a7611779030d

commit r9-8238-gf0a72494ea3747d1f66a0cda3e67a7611779030d
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Fri Feb 14 17:36:00 2020 +0100

    c++: Fix thinko in enum_min_precision [PR61414]
    
    When backporting the PR61414 fix to 8.4, I've noticed that the caching
    of prec is actually broken, as it would fail to actually store the computed
    precision into the hash_map's value and so next time we'd think the enum needs
    0 bits.
    
    2020-02-14  Jakub Jelinek  <jakub@redhat.com>
    
    	PR c++/61414
    	* class.c (enum_min_precision): Change prec type from int to int &.
    
    	* g++.dg/cpp0x/enum39.C: New test.
Comment 28 CVS Commits 2020-02-14 16:44:27 UTC
The releases/gcc-8 branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:53073523bca574251d45bded65b2b0c183b01e5d

commit r8-10022-g53073523bca574251d45bded65b2b0c183b01e5d
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Fri Feb 14 17:36:00 2020 +0100

    c++: Fix thinko in enum_min_precision [PR61414]
    
    When backporting the PR61414 fix to 8.4, I've noticed that the caching
    of prec is actually broken, as it would fail to actually store the computed
    precision into the hash_map's value and so next time we'd think the enum needs
    0 bits.
    
    2020-02-14  Jakub Jelinek  <jakub@redhat.com>
    
    	PR c++/61414
    	* class.c (enum_min_precision): Change prec type from int to int &.
    
    	* g++.dg/cpp0x/enum39.C: New test.
Comment 29 Jakub Jelinek 2020-02-14 17:22:42 UTC
Fixed for 8.4+ and 9.3+ too.
Comment 30 Eric Gallager 2020-02-16 17:49:36 UTC
(In reply to Jakub Jelinek from comment #29)
> Fixed for 8.4+ and 9.3+ too.

As far as I can tell, the commits in question just silenced the main false positives from that warning that people were complaining about, they didn't actually create a separate warning flag controlling the warning in question... I still think that's worth doing anyways.