[Support] Add LLVM_BITFIELD_WIDTH and LLVM_CHECKED_BITFIELD_ASSIGN macros.

[jlebar]

Summary

The latter lets us assign to a bitfield while also checking that the RHS
fits into the field.

There are many places where we check that the RHS "fits" before
assigning it to a bitfield; this lets us replace those with a single
line (which also correctly infers the size of the LHS).

[chandlerc]

You don't have any negative tests. Want to write some death tests behind NDEBUG?

llvm/include/llvm/Support/Bitfields.h

29

You're also relying on a bunch of other things here... Mostly that Type can be memset to all 1s without violating any of its invariants such as that this will create no trap representations.

I'm not sure how to word a static assert such that it catches this, but at the least you should check that it is standard layout and trivially copyable.

35–40

I don't think this will read effectively aftor auto-brief. I think you need the first sentence to be a prose description.

45

s/S/s/

llvm/unittests/Support/BitfieldsTest.cpp

23–24

That's odd, even with parentheses around the macro?

[jlebar]

In IRL conversation, we realized that this patch would be a lot simpler if we
relied on the fact that assigning a too-large value to an unsigned bitfield
field is guaranteed to truncate.

This gets around the assumptions I was making in the LLVM_BITFIELD_WIDTH macro,
that the target type is default-constructible and so on. It's also just less
code.

[jlebar]

Added death tests. Didn't realize we had those in LLVM.

llvm/unittests/Support/BitfieldsTest.cpp

23–24

EXPECT_EQ uses its args in unevaluated contexts, and that means we end up calling a lambda in an unevaluated context. :)

[rnk]

Do we really need this macro? I think we should just encourage this pattern:

class Foo {
private:
  unsigned Data : 12;
  ...
public:
  MyEnum getData() { return MyEnum(Data); }
  void setData(MyEnum V) { Data = V; assert(Data == V && "bitfield too small"); }
  ...
};

Sure, it's not compile time checked, but I hope we have tests exercising the use of all enums in our data structures.

[chandlerc]

In D23035#502855, @rnk wrote:

Do we really need this macro? I think we should just encourage this pattern:

class Foo {
private:
  unsigned Data : 12;
  ...
public:
  MyEnum getData() { return MyEnum(Data); }
  void setData(MyEnum V) { Data = V; assert(Data == V && "bitfield too small"); }
  ...
};

Sure, it's not compile time checked, but I hope we have tests exercising the use of all enums in our data structures.

I don't feel strongly about whether we should package this up in a macro and use the macro to implement such setData methods or not.

If folks want the macro, I'm much happier with the simpler incarnation.

[jlebar]

I think we should just encourage this pattern

Do you think it would be an improvement to rewrite clang's Expr.h to this pattern? Or, more to my point, D23036? It seems kind of wordy to me, but I'm open to thoughts.

I don't like that this macro obfuscates the code, but I do like that we don't have to rely on people writing correct check-in-range code. Like, it wasn't at all obvious to me that foo.x = y; assert(foo.x == y); was correct, or that it *wouldn't* be correct if the bitfield were signed. The macro gets that stuff right, which I like.

[rnk]

In D23035#502871, @jlebar wrote:

I think we should just encourage this pattern

Do you think it would be an improvement to rewrite clang's Expr.h to this pattern? Or, more to my point, D23036? It seems kind of wordy to me, but I'm open to thoughts.

You only end up using LLVM_CHECKED_BITFIELD_ASSIGN four times in that patch, so I'm not sure it's worth factoring the assertion out into a macro is worth it.

I don't like that this macro obfuscates the code, but I do like that we don't have to rely on people writing correct check-in-range code. Like, it wasn't at all obvious to me that foo.x = y; assert(foo.x == y); was correct, or that it *wouldn't* be correct if the bitfield were signed. The macro gets that stuff right, which I like.

Either way we have an education problem: either remember this library thing, or remember that round-tripping through the bitfield is the least fragile way to range check it. It's a *little* verbose, but there's enough less magic that I like writing it out a bit more.

[jlebar]

You only end up using LLVM_CHECKED_BITFIELD_ASSIGN four times in that patch, so I'm not sure it's worth factoring the assertion out into a macro is worth it.

And we only declare three non-single-bit bitfields -- that's a 1.3 calls per field! :)

Joking aside, a corollary of your point of strikes me as an argument against wrapping the bitfield's fields: Most bitfields have size 1 [1]. This idiom would force us to wrap all of them in getters/setters, or otherwise we'd have an inconsistent way of accessing the fields, even though we probably don't care about wrapping the size-one fields. With the macro, all reads work the same for all widths with the macro without requiring you to write five lines of boilerplate per field (when, like you point out, you only really want it for a few of the fields).

Similarly, if you have a bitfield struct with all size-one fields and then want to add a size-two field, the idiom would force you to rewrite all users.

Either way we have an education problem: either remember this library thing, or remember that round-tripping through the bitfield is the least fragile way to range check it.

I guess...remembering that a library exists isn't quite the same as remembering how to do it (and convincing your reviewers that you're right, and so on), though.

I don't want this to come across as though I'm totally sold on the macro. An option would be simply to get rid of the range checks in SelectionDAG, under the assumption that, if you add a value to one of these enums and then use it in SelectionDAG, hopefully you have a test that fails.

[1] Source: Perusing git grep ' : [0-9]\+\>' | grep -v '\?' | grep -v test

[jlebar]

Actually, better regexp: git grep ' : [0-9]\+;$' | grep -v '\?' | grep -v test

[jlebar]

ed, hit enter

An option would be simply to get rid of the range checks in SelectionDAG, under the assumption that, if you add a value to one of these enums and then use it in SelectionDAG, hopefully you have a test that fails.

In fact, we could even static_assert that e.g. ISD::LAST_LOADEXT_TYPE is <= 2**field_width.

One problem with this is we'd need to define a max field on AtomicOrdering. There's no great way to do that, because either the max == the last value, in which case it's inconsistent with basically every other enum max field, or otherwise the max == last_value + 1, in which case we morally lose strict typing on the enum class.

Still, I kind of like this solution, at least as far as storing enums, because it's checked at compile-time.

[jlebar]

OK, let's abandon this. I updated D23036 to the idiom as described here.

Thanks, everyone.

llvm/include/llvm/Support/Bitfields.h

+//===-- llvm/Support/BitFields.h - Utilities for C++ bit-fields -*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// Utilities for wrangling C++ bit-fields.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_SUPPORT_BIT_FIELDS_H
+#define LLVM_SUPPORT_BIT_FIELDS_H
+
+#include "llvm/Support/MathExtras.h"
+
+/// Gets the width in bits of Type::Field, which must be a bitfield with an
+/// underlying type that's an unsigned integer.
+///
+/// clang constant-folds this nicely, but other compilers (e.g. gcc) are not
+/// quite as smart.  So don't rely on this being free.
+#define LLVM_BITFIELD_WIDTH(Type, Field)                                       \
+  [] {                                                                         \
+    static_assert(::std::is_unsigned<decltype(Type::Field)>::value,            \
+                  #Type "::" #Field " must be an unsigned integral type.");    \
+    Type T;                                                                    \
+    ::memset(&T, 0xff, sizeof(T));                                             \
+    uint64_t Bits = T.Field;                                                   \
+    assert(::llvm::isMask_64(Bits) && "Error reading " #Type "::" #Field);     \
+    return ::llvm::countTrailingOnes(Bits);                                    \
+  }()
+
+/// Does
+///
+///   DstContainer.DstField = Src;
+///
+/// asserts that Src fits inside Dst.  DstField must be a bitfield with
+/// an unsigned integral underlying type.
+///
+/// For example:
+///
+/// \code
+///   Struct T {
+///     unsigned X : 2;
+///   };
+///
+///   T t;
+///   LLVM_CHECKED_BITFIELD_ASSIGN(t, X, 3);  // t.X = 3;
+///   LLVM_CHECKED_BITFIELD_ASSIGN(t, X, 42); // error: 42 doesn't fit in T::X.
+/// \endcode
+///
+#define LLVM_CHECKED_BITFIELD_ASSIGN(DstContainer, DstField, Src)              \
+  do {                                                                         \
+    auto V = (Src);                                                            \
+    assert(::llvm::isUIntN(                                                    \
+               LLVM_BITFIELD_WIDTH(decltype(DstContainer), DstField), V) &&    \
+           #Src " is too wide to fit inside " #DstContainer "::" #DstField);   \
+    DstContainer.DstField = V;                                                 \
+  } while (0)
+
+#endif

llvm/unittests/Support/BitfieldsTest.cpp

+//===- unittests/Support/MathExtrasTest.cpp - math utils tests ------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/Support/Bitfields.h"
+#include "gtest/gtest.h"
+
+namespace {
+
+TEST(BitfieldsTest, BitfieldWidth) {
+  struct S {
+    unsigned a : 1;
+    uint32_t b : 32;
+    uint64_t c : 64;
+    uint64_t d : 1;
+    uint64_t e : 10;
+  };
+  // Can't stick LLVM_BITFIELD_WIDTH macro into EXPECT_EQ -- the two macros'
+  // magics are at odds.
+  int V;
+  V = LLVM_BITFIELD_WIDTH(S, a);
+  EXPECT_EQ(1, V);
+  V = LLVM_BITFIELD_WIDTH(S, b);
+  EXPECT_EQ(32, V);
+  V = LLVM_BITFIELD_WIDTH(S, c);
+  EXPECT_EQ(64, V);
+  V = LLVM_BITFIELD_WIDTH(S, d);
+  EXPECT_EQ(1, V);
+  V = LLVM_BITFIELD_WIDTH(S, e);
+  EXPECT_EQ(10, V);
+}
+
+TEST(BitfieldsTest, BitfieldAssign) {
+  struct {
+    unsigned a : 1;
+    uint32_t b : 32;
+    uint64_t c : 64;
+  } S;
+
+  LLVM_CHECKED_BITFIELD_ASSIGN(S, a, 0);
+  EXPECT_EQ(0U, S.a);
+  LLVM_CHECKED_BITFIELD_ASSIGN(S, a, 1);
+  EXPECT_EQ(1U, S.a);
+
+  LLVM_CHECKED_BITFIELD_ASSIGN(S, b, std::numeric_limits<uint32_t>::max());
+  EXPECT_EQ(std::numeric_limits<uint32_t>::max(), S.b);
+  LLVM_CHECKED_BITFIELD_ASSIGN(S, b, 0);
+  EXPECT_EQ(0U, S.b);
+  LLVM_CHECKED_BITFIELD_ASSIGN(S, b, 42);
+  EXPECT_EQ(42U, S.b);
+
+  LLVM_CHECKED_BITFIELD_ASSIGN(S, c, std::numeric_limits<uint32_t>::max());
+  EXPECT_EQ(std::numeric_limits<uint32_t>::max(), S.c);
+  LLVM_CHECKED_BITFIELD_ASSIGN(S, c, std::numeric_limits<uint64_t>::max());
+  EXPECT_EQ(std::numeric_limits<uint64_t>::max(), S.c);
+  LLVM_CHECKED_BITFIELD_ASSIGN(S, c, 0);
+  EXPECT_EQ(0U, S.c);
+}
+
+} // namespace

llvm/unittests/Support/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Support
   )
 
 add_llvm_unittest(SupportTests
   AlignOfTest.cpp
   AllocatorTest.cpp
   ArrayRecyclerTest.cpp
+  BitfieldsTest.cpp
   BlockFrequencyTest.cpp
   BranchProbabilityTest.cpp
   Casting.cpp
   CommandLineTest.cpp
   CompressionTest.cpp
   ConvertUTFTest.cpp
   DataExtractorTest.cpp
   DwarfTest.cpp