[cfe-dev] Objective-C: methods, ivars, and cleanup

[David Chisnall]

On 25 Mar 2008, at 15:18, Devang Patel wrote:

> On Mar 24, 2008, at 11:48 AM, David Chisnall wrote:
>
>> Hi,
>>
>> This is a fairly big diff, so I've tried to split it into a few  
>> separate parts.
>>
>> globals.diff is a simple addition to the global_ctors code which  
>> avoids emitting empty global_ctors.
>
> -  // Populate the array
> -  std::vector<llvm::Constant*> CtorValues;
> -  llvm::Constant *MagicNumber =
> -    llvm::ConstantInt::get(llvm::Type::Int32Ty, 65535, false);
> -  std::vector<llvm::Constant*> StructValues;
> -  for (std::vector<llvm::Constant*>::iterator I =  
> GlobalCtors.begin(),
> -       E = GlobalCtors.end(); I != E; ++I) {
> -    StructValues.clear();
> -    StructValues.push_back(MagicNumber);
> -    StructValues.push_back(*I);
> +    // Populate the array
> +    std::vector<llvm::Constant*> CtorValues;
> +    llvm::Constant *MagicNumber =  
> llvm::ConstantInt::get(llvm::IntegerType::Int32Ty,
> +        65535,
> +        false);
> +    for (std::vector<llvm::Constant*>::iterator I =  
> GlobalCtors.begin(),
> +        E = GlobalCtors.end(); I != E; ++I) {
> +      std::vector<llvm::Constant*> StructValues;
> +      StructValues.push_back(MagicNumber);
>
> Hoist StructValues outside the for loop and use clear(), as it was  
> done originally. Looks OK otherwise.

Fixed.

>>
>>
>> etoile_runtime.diff contains a subclass of CGObjCRuntime which  
>> implements the runtime-specific parts for the Étoilé runtime.   
>> Other parts do not depend on this unless output for this runtime is  
>> selected (the runtime to use is currently hard-coded).  I am now  
>> writing the GNU and Étoilé versions at the same time to make sure  
>> the interfaces are sufficiently abstract.  Hopefully this will make  
>> adding support for the Apple runtimes easy for whoever gets that job.
>
> OK. Few nit-picks
>
> 1) Add doxygen style comments for class methods and variables. Right  
> now, I don't see any comment.

Done.

> 2)
> +#define SET(structure, index, value) do {\
> +    llvm::Value *element_ptr = Builder.CreateStructGEP(structure,  
> index);\
> +    Builder.CreateStore(value, element_ptr);} while(0)
>
> Avoid macros like this.

Can you suggest an alternative?  I use this pattern in a really large  
number of places (and will use it in a lot more by the time I'm  
finished.  Expanding it everywhere makes the code a lot less readable  
and is likely to cause copy-and-paste bugs.

> 3)
> +// BIG FAT WARNING: Much of this code will need factoring out later.
>
> Just use, FIXME: Much of this code ...

Done.


> 4) Now, I regularly see one-parameter-per-line in your patches. For  
> example,
>
> +  // Slot type
> +  SlotTy = llvm::StructType::get(IntTy,
> +      IMPTy,
> +      PtrToInt8Ty,
> +      PtrToInt8Ty,
> +      llvm::Type::Int32Ty,
> +      0);
>
> I'd avoid this myself. Clang coding style does not require this (nor  
> explicitly prohibits this).

I've only done this where putting them all on the same line would make  
the line over 80 characters long.  I find it more readable than just  
wrapping at 80 characters.

>> obj_struct.diff handles conversion from Objective-C pointers to  
>> LLVM types.  This is needed for accessing instance variables  
>> statically and is used in the ivar access parts.  This patch is  
>> also self-contained, and does not depend on any other changes.
>
>
> +  void CollectIvarTypes(ObjCInterfaceDecl *ObjCClass,
> +      std::vector<const llvm::Type*> *IvarTypes);
>
> This is Objective-C specific class method, so rename it as  
> CollectObjCIvarTypes.
> Prefer SmallVector instead of std::vector. Pass it as a reference  
> instead of a pointer.

Fixed passing by pointer.  Kept as std::vector because  
StructType::get() takes a std::vector.

>   /// getLLVMFieldNo - Return llvm::StructType element number
>   /// that corresponds to the field FD.
>   unsigned getLLVMFieldNo(const FieldDecl *FD);
> +  unsigned getLLVMFieldNo(QualType ObjectTy, const ObjCIvarDecl  
> *Decl);
>
> Why do you need new method here ?

This gets the field number from an LLVM structure created from an  
Objective-C class, rather than one created from a C structure.   
Possibly it should have a different name?  Suggestions welcome.

> +    // Warning: Use of this is strongly discouraged.  Late binding  
> of instance
> +    // variables is supported on some runtimes and so using static  
> binding can
> +    // break code when libraries are updated.  Only use this if you  
> have
> +    // previously checked that the ObjCRuntime subclass in use does  
> not support
> +    // late-bound ivars.
>
> Please add assert() to check late-bound of ivars support.

I'd like to do this, but GodeGenTypes does not have a reference to  
anything that has a reference to anything that has a reference to the  
ObjCRuntime subclass, so it's non-trivial.  Suggestions welcome.

>> runtime_if.diff contains changes to the runtime interface.  This  
>> probably needs to be committed at the same time as the changes to  
>> the GNU runtime, or there might be some conflicts.  The Étoilé  
>> patch may also conflict with the old definitions if it is committed  
>> before this.
>
> OK
>
> -CGObjCRuntime *CreateObjCRuntime(llvm::Module &M);
> +CGObjCRuntime *CreateObjCRuntime(llvm::Module &M,
> +                                 const llvm::Type *LLVMIntType,
> +                                 const llvm::Type *LLVMLongType);
>
> Is not it possible to derive LLVMIntType and LLVMLongType info based  
> on Module ?

Maybe?  Can I have a hint please?

>> gnu_runtime.diff contains the changes made to the GNU runtime to  
>> adopt the new interface.  It also adds methods for generating  
>> method functions and some things related to creating class  
>> structures which are not used yet.
>
> OK. Please add doxygen style comments for each method.

Is three slashes right for Doxygen?

>>
>>
>> method_gen.diff contains the code required to generate Objective-C  
>> methods (although it does not yet attach them to class structures.
>
> Index: lib/CodeGen/CGStmt.cpp
> ===================================================================
> --- lib/CodeGen/CGStmt.cpp	(revision 48734)
> +++ lib/CodeGen/CGStmt.cpp	(working copy)
> @@ -332,9 +332,6 @@
>   // Emit the result value, even if unused, to evalute the side  
> effects.
>   const Expr *RV = S.getRetValue();
>
> -  QualType FnRetTy = CurFuncDecl->getType().getCanonicalType();
> -  FnRetTy = cast<FunctionType>(FnRetTy)->getResultType();
> -
>   if (FnRetTy->isVoidType()) {
>     // If the function returns void, emit ret void.
>     Builder.CreateRetVoid();
>
> Is this intentional ?

Yes.  These have been moved to instance methods variables and created  
earlier because they can not be inferred in the same way for Objective- 
C methods as for C functions.

> +  // Skip the hidden arguments.
> +  ++AI; ++AI;
>
> Name two hidden arguments in the comment.

Done.

> void CodeGenFunction::GenerateCode(const FunctionDecl *FD) {
>   LLVMIntTy = ConvertType(getContext().IntTy);
>   LLVMPointerWidth = static_cast<unsigned>(
>      
> getContext 
> ().getTypeSize(getContext().getPointerType(getContext().VoidTy)));
>
>   CurFuncDecl = FD;
> +  FnRetTy = CurFuncDecl->getType().getCanonicalType();
> +  FnRetTy = cast<FunctionType>(FnRetTy)->getResultType();
> +
>
> Is this intentional ?

Yes.  See above.  These are set in GenerateCode and GenerateObjCMethod  
now, so that they are the only two methods that need to know whether a  
C function or an Objective-C method is being generated.

>> Finally, ivar.diff contains the code required for accessing and  
>> modifying Objective-C instance variables.  This depends on the  
>> stuff in objc_struct.diff for all runtimes that do not use late- 
>> bound instance variable addresses (i.e. all of the currently  
>> supported ones).
>
> @@ -341,6 +345,17 @@
>       return LValue::MakeAddr(CGM.GetAddrOfGlobalVar(VD, false));
>     else {
>       llvm::Value *V = LocalDeclMap[D];
> +      // Check if it's an implicit argument
> +      if (!V) {
> +        std::string VarName(D->getName());
> +        llvm::Function::arg_iterator AI = CurFn->arg_begin();
> +        while(AI) {
> +          if(VarName == AI->getName()) {
> +            return LValue::MakeAddr(AI);
> +          }
> +        }
> +      }
>       assert(V && "BlockVarDecl not entered in LocalDeclMap?");
>       return LValue::MakeAddr(V);
>     }
>
> I'm getting dense here. Please explain what is "implicit argument"  
> in this code? Thanks!

In Objective-C, a method is declared like this:

- (id) aMethodTakingAnInteger:(int)anInt andAnObject:(id)anObject

This is then converted to a C function like this:

id method(id self, SEL _cmd, int anInt, id anObject)

The two first arguments are implicit - they are not specified  
explicitly in the code, but are inserted in both the caller and callee  
by the compiler.  They are not always self and _cmd (they are in the  
GNU and Apple runtimes, but not in the Étoilé one).

On 25 Mar 2008, at 16:01, Chris Lattner wrote:
>
>> globals.diff is a simple addition to the global_ctors code which  
>> avoids emitting empty global_ctors.
>
> A couple things here:
>
> Instead of nesting the entire body of the function, please just use:
>
> if (GlobalCtors.empty()) return;

Done.

> Second, you're implicitly reverting a cleanup fix:
>
> -  llvm::StructType* CtorStructTy =
> -  llvm::StructType::get(llvm::Type::Int32Ty, FPType, NULL);
>
> is better than:
>
> +    std::vector<const llvm::Type*> CtorFields;
> +    CtorFields.push_back(llvm::IntegerType::get(32));
> +    // i32, function type pair
> +    CtorFields.push_back(llvm::PointerType::getUnqual(CtorFuncTy));
> +    llvm::StructType* CtorStructTy =  
> llvm::StructType::get(CtorFields, false);
>
> Likewise the fix to inline GlobalCtorsVar and several other things.
>
> Finally, please make sure the code fits in 80 columns.


Ooops.  svn said it was a conflict when I updated and I thought it was  
just being wrong.  I've reverted this to the version in svn and  
reapplied the fixes and now it is more sensible.


>> etoile_runtime.diff contains a subclass of CGObjCRuntime which  
>> implements the runtime-specific parts for the Étoilé runtime.   
>> Other parts do not depend on this unless output for this runtime is  
>> selected (the runtime to use is currently hard-coded).  I am now  
>> writing the GNU and Étoilé versions at the same time to make sure  
>> the interfaces are sufficiently abstract.  Hopefully this will make  
>> adding support for the Apple runtimes easy for whoever gets that job.
>
> Ok. I'll let you worry about the correctness of this code :).   
> Please make sure you follow coding conventions though.  For example,  
> don't include commented out code:
>
> +/*
> +clang::CodeGen::CGObjCRuntime *clang::CodeGen::CreateObjCRuntime(
> +    llvm::Module &M,
> +    const llvm::Type *LLVMIntType,
> +    const llvm::Type *LLVMLongType) {
> +  return new CGObjCEtoile(M, LLVMIntType, LLVMLongType);
> +}
> +*/

Ooops.  I just have that for testing.  At some point I need to add a  
command-line option for selecting the runtime so I don't need a  
recompile every time I want to switch.  Want to give me any hints  
about which bits of code I need to look at to do this?

> Please fit in 80 cols, please don't use macros:
>
> +#define SET(structure, index, value) do {\
> +    llvm::Value *element_ptr = Builder.CreateStructGEP(structure,  
> index);\
> +    Builder.CreateStore(value, element_ptr);} while(0)
>
> (use a static function instead)

Fixed.

> Stuff like this won't work on 64-bit platforms:
>
>  llvm::Constant *SelFunction =
> +    TheModule.getOrInsertFunction("lookup_typed_selector",
> +        SelectorTy,
> +        PtrToInt8Ty,
> +        PtrToInt8Ty,
> +        0);
> +
>
> Use NULL instead of 0.  Likewise for StructType::get and other  
> variadic functions.

Fixed everywhere I spotted it.  Let me know if I've missed any...

> +#include <stdarg.h>

Ooops.  That's left over from a helper function that's now in LLVM.

> I don't think you need this, but if you do, please use <cstdarg>  
> instead.
>
> +//===------- CGObjCEtoile.cpp - Emit LLVM Code from ASTs for a  
> Module --------===//
>
> This should follow the formating of the other files more closely.

Slightly confused - I copied the formatting directly from one of the  
other files...

The attached diff contains all of the changes mentioned in this email,  
and a few minor cleanups, but no completely new code.

David

-------------- next part --------------
A non-text attachment was scrubbed...
Name: clang.diff
Type: application/octet-stream
Size: 46666 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20080325/279df6ed/attachment.obj>
-------------- next part --------------