[llvm-commits] [llvm] r150507 - in /llvm/trunk: include/llvm/CodeGen/TargetLoweringObjectFileImpl.h include/llvm/Target/TargetLoweringObjectFile.h lib/CodeGen/AsmPrinter/AsmPrinter.cpp lib/CodeGen/TargetLoweringObjectFileImpl.cpp

[Bill Wendling]

On Feb 15, 2012, at 3:59 AM, Chris Lattner <clattner at apple.com> wrote:

> On Feb 14, 2012, at 1:28 PM, Bill Wendling wrote:
> 
>> Author: void
>> Date: Tue Feb 14 15:28:13 2012
>> New Revision: 150507
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=150507&view=rev
>> Log:
>> Add code to the target lowering object file module to handle module flags.
>> 
>> The MachO back-end needs to emit the garbage collection flags specified in the
>> module flags. This is a WIP, so the front-end hasn't been modified to emit these
>> flags just yet. Documentation and front-end switching to occur soon.
> 
> Looks nice, some comments:
> 
> +  // Emit module flags.
> +  if (NamedMDNode *ModFlags = M.getModuleFlagsMetadata())
> +    getObjFileLowering().emitModuleFlags(OutStreamer, ModFlags, Mang, TM);
> ...
> 
> +  /// emitModuleFlags - Emit the module flags that specify the garbage
> +  /// collection information.
> +  virtual void emitModuleFlags(MCStreamer &Streamer, NamedMDNode *ModFlags,
> +                               Mangler *Mang, const TargetMachine &TM) const;
> 
> Instead of passing the NamedMDNode down and having all clients have to tear it apart, it would be better for a Module:: api to decompose them into structs and then pass an ArrayRef of structs into TLOF.  Please add a new local struct to Module that contains "ModAttrBehavior Behavior, StringRef Key, Value *Val", and add method to Module that returns the (possibly empty) list for the module.
> 
> That would make the AsmPrinter.cpp common code look like:
> 
> SmallVector<Module::ModuleFlagEntry, 8> ModuleFlags;
> M.getModuleFlags(ModuleFlags);
> if (!ModuleFlags.empty()) 
>  ... emitModuleFlags(... ModuleFlags, ...)
> 
Okay. That sounds good.

> 
>> +void TargetLoweringObjectFileMachO::
>> +emitModuleFlags(MCStreamer &Streamer, NamedMDNode *ModFlags,
>> +                Mangler *Mang, const TargetMachine &TM) const {
>> +  StringRef Version("Objective-C Image Info Version");
>> +  StringRef SectionSpec("Objective-C Image Info Section");
>> +  StringRef GC("Objective-C Garbage Collection");
>> +  StringRef GCOnly("Objective-C GC Only");
> 
> Is there a specific reason you factored these out like this?  Why not just do 
> 
> if (KeyStr == "Objective-C Image Info Version")  down below?
> 
In my mind there was a reason, but it's not really necessary (and isn't valid for this code). :-)

>> +  // The section is mandatory. If we don't have it, then we don't have image
>> +  // info information.
>> +  if (Section.empty()) return;
>> +
>> +  uint32_t Values[2] = { VersionVal, GCFlags };
>> +  Module *M = ModFlags->getParent();
>> +  Constant *Init = ConstantDataArray::get(M->getContext(), Values);
>> +  GlobalVariable *GV = new GlobalVariable(*M, Init->getType(), false,
>> +                                          GlobalValue::InternalLinkage, Init,
>> +                                          "\01L_OBJC_IMAGE_INFO");
>> +  GV->setSection(Section);
>> +
>> +  MCSymbol *GVSym = Mang->getSymbol(GV);
>> +  SectionKind GVKind = TargetLoweringObjectFile::getKindForGlobal(GV, TM);
>> +  const MCSection *TheSection = SectionForGlobal(GV, GVKind, Mang, TM);
> 
> This should not be creating (and leaking :) a temporary GlobalVariable.  Just drive the Streamer API directly to get the directives you need.
> 
Doh!! Sorry for the leakage. I'll modify it to directly emit things.

Thanks for the review!

-bw