[builtin] Add bitfield support for __builtin_dump_struct

[paulsemel]

Summary

This is an attempt for adding bitfield support to __builtin_dump_struct.

[paulsemel]

This version is working for non packed structures. For packed structures, it might sometimes work, sometimes not.
The resulting assembly code seems to be the right one.
If someone went through bitfields manipulation, please do not hesitate to comment !
Requesting for help :)

[paulsemel]

ping :) @aaron.ballman

[lebedev.ri]

This seems to be missing tests.

[aaron.ballman]

lib/CodeGen/CGBuiltin.cpp

1250

What happens if this overflows due to being < 0?

[paulsemel]

I will for sure add tests @lebedev.ri . Fact is that, for the moment, this is not working as expected.
This is why I am asking for a bit of help about this bitfield handling :)

lib/CodeGen/CGBuiltin.cpp

1250

How could this be < 0 ?
If it is, it means that there was a problem during bitfield construction isn't it ?
I mean StorageSize is always greater that Offset + Size

[aaron.ballman]

In D47953#1137375, @paulsemel wrote:

I will for sure add tests @lebedev.ri . Fact is that, for the moment, this is not working as expected.
This is why I am asking for a bit of help about this bitfield handling :)

Adding a few more reviewers to see if they have ideas on the bitfield handling. Can you describe a bit more about what's incorrect with it when the structures are packed? Code examples might help.

lib/CodeGen/CGBuiltin.cpp

1250

I don't think it should happen in the normal course of things. I'd probably just put in an assert to ensure that the high bit is never set in HighBits unless it's also set in StorageSize.

[dblaikie]

Yeah, I know nothing about this dump feature or what's being fixed here - test cases would be great to help motivate/explain.

[paulsemel]

First thanks all for reviewing !

Basically, what's happening is that it works good with non-packed structures.
Here is an example for packed structure (with unsigned signed short):

struct lol {
	unsigned short a:3;
	unsigned short b:2;
	unsigned short c:6;
	unsigned short d:2;
	unsigned short e:6;
} __attribute__((packed));

int main(void)
{
	struct lol lolo = {
		.a = 2,
		.b = 1,
		.c = 13,
		.d = 2,
		.e = 9
	};
	__builtin_dump_struct(&lolo, &printf);
	return 0;
}

Here is the output I get :

unsigned short a : 2
unsigned short b : 1
unsigned short c : 13
unsigned short d : 2
unsigned short e : 1

The last unsigned short is incorrect.

If I switch this to signed integers, I get :

unsigned short a : 0
unsigned short b : 0
unsigned short c : 0
unsigned short d : 0
unsigned short e : 0

All the fields are zeroed. I absolutely do not understand what is happening...
Thanks for trying to help guys !

[dblaikie]

This doesn't seem to build for me - so hard to debug/probe it:

llvm/src/tools/clang/lib/CodeGen/CGBuiltin.cpp:1264:65: error: no viable conversion from 'clang::QualType' to 'llvm::Type *'

CGF.CGM.getDataLayout().getTypeSizeInBits(CanonicalType),
                                          ^~~~~~~~~~~~~

llvm/src/include/llvm/IR/DataLayout.h:560:53: note: passing argument to parameter 'Ty' here
inline uint64_t DataLayout::getTypeSizeInBits(Type *Ty) const {

^

1 error generated.

[paulsemel]

Fixed version problem. Now building.

[paulsemel]

In D47953#1143044, @dblaikie wrote:

This doesn't seem to build for me - so hard to debug/probe it:

llvm/src/tools/clang/lib/CodeGen/CGBuiltin.cpp:1264:65: error: no viable conversion from 'clang::QualType' to 'llvm::Type *'

CGF.CGM.getDataLayout().getTypeSizeInBits(CanonicalType),
                                          ^~~~~~~~~~~~~

llvm/src/include/llvm/IR/DataLayout.h:560:53: note: passing argument to parameter 'Ty' here
inline uint64_t DataLayout::getTypeSizeInBits(Type *Ty) const {

^

1 error generated.

Very sorry about it, I should have paid more attention..

[dblaikie]

Yeah, doesn't look like this code handles a value crossing the boundary of
the size of the bitfield type (it's reading only the low bit).

I suspect looking at the code that generates bitfield accesses would be
useful - and/or maybe actually calling into that very code, rather than
reimplementing it here? CodeGenFunction::EmitLoadOfBitfieldLValue seems to
be the place to go (I found this by writing a short example that loads one
of these strided bitfield values & then breaking in
llvm::BinaryOperator::BinaryOperator until I found the 'and' operation,
since that seemed like the more interesting one).

lib/CodeGen/CGBuiltin.cpp

 static llvm::Value *dumpRecord(CodeGenFunction &CGF, QualType RType,
                                Value *&RecordPtr, CharUnits Align, Value *Func,
                                int Lvl) {
   const auto *RT = RType->getAs<RecordType>();
   ASTContext &Context = CGF.getContext();
   RecordDecl *RD = RT->getDecl()->getDefinition();
   ASTContext &Ctx = RD->getASTContext();
   const ASTRecordLayout &RL = Ctx.getASTRecordLayout(RD);
+  const CGRecordLayout &CGRL = CGF.getTypes().getCGRecordLayout(RD);
   std::string Pad = std::string(Lvl * 4, ' ');
 
   Value *GString =
       CGF.Builder.CreateGlobalStringPtr(RType.getAsString() + " {\n");
   Value *Res = CGF.Builder.CreateCall(Func, {GString});
 
   static llvm::DenseMap<QualType, const char *> Types;
   if (Types.empty()) {
for (const auto *FD : RD->fields()) {
     Off = Ctx.toCharUnitsFromBits(Off).getQuantity();
 
     Value *FieldPtr = RecordPtr;
     if (RD->isUnion())
       FieldPtr = CGF.Builder.CreatePointerCast(
           FieldPtr, CGF.ConvertType(Context.getPointerType(FD->getType())));
     else
       FieldPtr = CGF.Builder.CreateStructGEP(CGF.ConvertType(RType), FieldPtr,
-                                             FD->getFieldIndex());
+                                             CGRL.getLLVMFieldNo(FD));
 
     GString = CGF.Builder.CreateGlobalStringPtr(
         llvm::Twine(Pad)
             .concat(FD->getType().getAsString())
             .concat(llvm::Twine(' '))
             .concat(FD->getNameAsString())
             .concat(" : ")
             .str());
if (CanonicalType->isRecordType()) {
       continue;
     }
 
     // We try to determine the best format to print the current field
     llvm::Twine Format = Types.find(CanonicalType) == Types.end()
                              ? Types[Context.VoidPtrTy]
                              : Types[CanonicalType];
 
+    if (FD->isBitField())
+      FieldPtr = CGF.Builder.CreatePointerCast(
+          FieldPtr, CGF.ConvertType(Context.getPointerType(CanonicalType)));
+
     Address FieldAddress = Address(FieldPtr, Align);
     FieldPtr = CGF.Builder.CreateLoad(FieldAddress);
 
-    // FIXME Need to handle bitfield here
+    if (FD->isBitField()) {
+      const CGBitFieldInfo &Info = CGRL.getBitFieldInfo(FD);
+      if (Info.IsSigned) {
+        unsigned HighBits = Info.StorageSize - Info.Offset - Info.Size;
+        if (HighBits)
+          FieldPtr = CGF.Builder.CreateShl(FieldPtr, HighBits);
+        if (Info.Offset + HighBits)
+          FieldPtr = CGF.Builder.CreateAShr(FieldPtr, Info.Offset + HighBits);
+      } else {
+        if (Info.Offset)
+          FieldPtr = CGF.Builder.CreateLShr(FieldPtr, Info.Offset);
+        if (static_cast<unsigned>(Info.Offset) + Info.Size < Info.StorageSize)
+          FieldPtr = CGF.Builder.CreateAnd(
+              FieldPtr,
+              ConstantInt::get(CGF.ConvertType(CanonicalType),
+                               llvm::APInt::getLowBitsSet(
+                                   CGF.CGM.getDataLayout().getTypeSizeInBits(
+                                       CGF.ConvertType(CanonicalType)),
+                                   Info.Size)));
+      }
+      FieldPtr = CGF.Builder.CreateIntCast(
+          FieldPtr, CGF.ConvertType(CanonicalType), Info.IsSigned);
+    }
+
     GString = CGF.Builder.CreateGlobalStringPtr(
         Format.concat(llvm::Twine('\n')).str());
     TmpRes = CGF.Builder.CreateCall(Func, {GString, FieldPtr});
     Res = CGF.Builder.CreateAdd(Res, TmpRes);
   }
 
   GString = CGF.Builder.CreateGlobalStringPtr(Pad + "}\n");
   Value *TmpRes = CGF.Builder.CreateCall(Func, {GString});