Re: [PATCH] IB/core: Fix different types mix in ib_device_cap_flags structure values

[Leon Romanovsky <leon-2ukJVAZIZ/Y@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2100 bytes --]

On Tue, May 31, 2016 at 11:13:06AM -0600, Jason Gunthorpe wrote:
> On Tue, May 31, 2016 at 08:35:10AM -0700, Bart Van Assche wrote:
> > On 05/30/16 03:09, Max Gurtovoy wrote:
> > >ib_device_cap_flags 64-bit expansion caused a possible caps overlapping
> > >(depending on machine endianness) and made consumers read wrong device
> > >capabilities. For example IB_DEVICE_SG_GAPS_REG was falsely read by the
> > >iser driver causing it to use a non-existing capability. Fix this by
> > >casting ib_device_cap_flags enumerations to ULL.
> > >
> > > [ ... ]
> > >diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> > >[ ... ]
> > > enum ib_device_cap_flags {
> > >       [ ... ]
> > > 	IB_DEVICE_SG_GAPS_REG			= (1ULL << 32),
> > >      [ ... ]
> > > };
> > 
> > How can this patch make a difference? The presence of any constant
> > in an enum that does not fit in a 32-bit integer makes an enum 64
> > bits wide. In other words, all the changes from "1" into "1ULL" in
> > this patch do not have
> 
> The expressions are evaluated before the enum type is decided, the
> enum type has no impact on the type of the expressions.

It is machine/compiler dependent.

Bart,
Can you share your source of C-standard?

This link [1] states in chapter "6.7.2.2 Enumeration specifiers"

"Each enumerated type shall be compatible with char, a signed integer type,
or an unsigned integer type. The choice of type is implementation-defined (110),
but shall be capable of representing the values of all the members of the enumeration.
The enumerated type is incomplete until after the } that terminates the list of enumerator
declarations."

And the footnote (110):
"An implementation **may** delay the choice of which integer type until all enumeration
constants have been seen."

[1] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf


> 
> (1<<32) is always undefined behavior because '1' is only a 32 bit type.
> 
> I'm confused why we didn't get any static checker hits on the shift
> overflow - modern compilers warn on that??
> 
> Jason

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]