[cfe-dev] Patch proposal for __builtin_offsetof construct.

[Douglas Gregor]

On Apr 22, 2010, at 12:30 PM, Roberto Amadini wrote:

> Douglas Gregor ha scritto:
>> On Apr 16, 2010, at 3:35 AM, Roberto Amadini wrote:
>> 
>>  
>>> Hi,
>>> 
>>> I'm the student who is trying to fix the problem mentioned in
>>> http://lists.cs.uiuc.edu/pipermail/cfe-dev/2010-February/007833.html .
>>>    
> [...]
>>> Index: include/clang/AST/Expr.h
>>> ===================================================================
>>> --- include/clang/AST/Expr.h	(revisione 100962)
>>> +++ include/clang/AST/Expr.h	(copia locale)
>>> @@ -991,6 +985,78 @@
>>>    
> [...]
>>> +public:
>>> +  // __builtin_offsetof(type, identifier(.identifier|[expr])*)
>>> +  // FIXME: Type-dependent case ignored.
>>> +  struct OffsetOfNode {
>>> +    SourceLocation LocStart, LocEnd;
>>> +    bool isBrackets;  // true if [expr], false if .ident
>>> +    union {
>>> +      FieldDecl *MemberDecl;
>>> +      Expr *IndexExpr;
>>> +    } U;
>>> +  };
>>>    
>> 
>> You could save a word of storage per OffsetOfNode by mangling the "isBrackets" bit into the lowest bit of the FieldDecl or Expr pointer. llvm::PointerUnion<FieldDecl *, Expr *> will do this for you.
>> 
>> But, see below for more comments on this representation.
>>  
> In Order to have Expr*'s in contiguous memory, I replaced *IndexExpr
> field with an unsigned integer ExprIndex, which represents the index of the
> corresponding Expr* in the array of subscript expressions: in this way, I can't
> use a PointerUnion, or not?

Correct. PointerUnion won't help here. Instead, I played more fun tricks by using the lower two bits of a pointer-sized integer as a descriminator. For a FieldDecl*, we mask off those bits and cast the type; for an expression index, we shift away those bits.

>>> Index: lib/AST/StmtProfile.cpp
>>> ===================================================================
>>> --- lib/AST/StmtProfile.cpp	(revisione 100962)
>>> +++ lib/AST/StmtProfile.cpp	(copia locale)
>>> @@ -261,6 +261,10 @@
>>>  ID.AddInteger(S->getOpcode());
>>> }
>>> 
>>> +void StmtProfiler::VisitOffsetOfExpr(OffsetOfExpr *S) {
>>> +  VisitExpr(S);
>>> +}
>>> +
>>>    
>> 
>> To profile an OffsetOfExpr, we'll need to visit the type and all of the OffsetOfNodes as well. That way, we can distinguish between different OffsetOfExprs on the same base type.
>> 
>>  
> Ok, but I have a doubt: VisitExpr(S) visits recursively all the sub-expressions,
> thus I think that I haven't to explicitly visit such expressions while I'm iterating on
> OffsetOfNodes array for declarations visit... Or not?

Right. VisitStmt(S) will get the subexpressions, but it won't get the field declarations.


>> Overall, this is a great start on __builtin_offsetof, thanks! I look forward to seeing a revised patch.
>> 
>> 	- Doug
>>  
> Thanks for your comments!  I tried to follow your advices...
> I attached a new patch which refers to revision no. 102050.


This is great. I've committed a modified version of your patch, where I compressed the storage of OffsetOfNode a bit and added support for template instantiation and precompiled headers. Since we already had __builtin_offsetof that somewhat worked, everything had to go in at once. Here's what I ended up doing:

	- Implemented the bit-mangled encoding I described above, to shrink the size of OffsetOfNode. Added accessors/constructors to OffsetOfNode, so that the representation is hidden.
	- Simplified the pointer arithmetic inside OffsetOfExpr.
	- Added support for templates:
		- OffsetOfNode now has an "Identifier", for the case where the offsetof designates a field but we can't look up the field yet (since the record type is dependent).
		- Reorganized semantic checking so that it copes with dependent types.
		- Added template instantiation logic
	- Added support for reading/writer OffsetOfExpr to precompiled headers

The commit is here:

	http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20100426/029847.html
	
Thanks for working on this!

	- Doug