Messages in this thread | | | From | Rikard Falkeborn <> | Date | Tue, 6 Aug 2019 21:27:27 +0200 | Subject | Re: [PATCH] linux/bits.h: Add compile time sanity check of GENMASK inputs |
| |
Hi Masahiro,
On Wed, Aug 07, 2019 at 12:19:36AM +0900, Masahiro Yamada wrote: > Hi Rikard, > > > On Tue, Aug 6, 2019 at 4:55 AM Rikard Falkeborn > <rikard.falkeborn@gmail.com> wrote: > > > > On Sun, Aug 04, 2019 at 03:45:16PM +0900, Masahiro Yamada wrote: > > > On Sun, Aug 4, 2019 at 3:36 AM Rikard Falkeborn > > > <rikard.falkeborn@gmail.com> wrote: > > > > > > > > On Sat, Aug 03, 2019 at 12:12:46PM +0900, Masahiro Yamada wrote: > > > > > On Sat, Aug 3, 2019 at 12:03 PM Masahiro Yamada > > > > > <yamada.masahiro@socionext.com> wrote: > > > > > > > > > > > > > > > > > BTW, v2 is already inconsistent. > > > > > > If you wanted GENMASK_INPUT_CHECK() to return 'unsigned long',, > > > > > > you would have to cast (low) > (high) as well: > > > > > > > > > > > > (unsigned long)((low) > (high)), UL(0)))) > > > > > > > > > > > > This is totally redundant, and weird. > > > > > > > > > > I take back this comment. > > > > > You added (unsigned long) to the beginning of this macro. > > > > > So, the type is consistent, but I believe all casts should be removed. > > > > > > > > Maybe you're right. BUILD_BUG_ON_ZERO returns size_t regardless of > > > > inputs. I was worried that on some platform, size_t would be larger than > > > > unsigned long (as far as I could see, the standard does not give any > > > > guarantees), and thus all of a sudden GENMASK would be 8 bytes instead > > > > of 4, but perhaps that is not a problem? > > > > > > > > > How about adding (int) cast to BUILD_BUG_ON_ZERO() ? > > > > I'll have a look. > > > > I found a more important problem in this patch. > > You used __is_constexpr(), which is defined in <linux/kernel.h>. > > This header does not include <linux/kernel.h>, > so this header is not self-contained anymore. > > The following test code fails to build: > > #include <linux/bits.h> > unsigned long foo(unsigned long in_bits) > { > return in_bits & GENMASK(5, 3); > } > > > > However, you cannot include <linux/kernel.h> from <linux/bits.h>. > See the log of 8bd9cb51daac89337295b6f037b0486911e1b408 > > This header was split out to not pull in <linux/bitops.h> > Including <linux/kernel.h> pulls in <linux/bitops.h> again. > > > In summary, please use __builtin_constant_p() > instead of __is_constexpr(). > > > You can shorten __builtin_constant_p(high) && __builtin_constant_p(low) > into __builtin_constant_p((low) > (high)). > > > How about this? > > > #define GENMASK_INPUT_CHECK(high, low) \ > BUILD_BUG_ON_ZERO(__builtin_choose_expr( \ > __builtin_constant_p((low) > (high)), (low) > (high), 0)) > > > -- > Best Regards > Masahiro Yamada
Thanks for the feedback, your version looks much cleaner than mine. I *think* I had a reason for using __is_constexpr() instead of __builtin_constant_p but I'll try a full rebuild to see if something comes up.
Best Regards, Rikard Falkeborn
| |