[LLVMdev] Win32 COFF Support - Patch 3

[Daniel Dunbar]

Hi Michael,

Patch looks excellent, one minor comment:
--
>  void MCMachOStreamer::EmitLabel(MCSymbol *Symbol) {
> +  // TODO: This is exactly the same as WinCOFFStreamer. Consider merging into
> +  // MCObjectStreamer.

This shouldn't be true, comments below.

>  void WinCOFFStreamer::EmitLabel(MCSymbol *Symbol) {
> +  // TODO: This is copied exactly from the MachOStreamer. Consider merging into
> +  // MCObjectStreamer?
> +  assert(Symbol->isUndefined() && "Cannot define a symbol twice!");
> +  assert(!Symbol->isVariable() && "Cannot emit a variable symbol!");
> +  assert(CurSection && "Cannot emit before setting section!");
> +
> +  Symbol->setSection(*CurSection);
> +
> +  MCSymbolData &SD = getAssembler().getOrCreateSymbolData(*Symbol);
> +
> +  // We have to create a new fragment if this is an atom defining symbol,
> +  // fragments cannot span atoms.
> +  if (getAssembler().isSymbolLinkerVisible(SD.getSymbol()))
> +    new MCDataFragment(getCurrentSectionData());

This is a Darwin specific thing, on Windows I believe you can remove
this clause.

> +  // FIXME: This is wasteful, we don't necessarily need to create a data
> +  // fragment. Instead, we should mark the symbol as pointing into the data
> +  // fragment if it exists, otherwise we should just queue the label and set its
> +  // fragment pointer when we emit the next fragment.
> +  MCDataFragment *F = getOrCreateDataFragment();
> +  assert(!SD.getFragment() && "Unexpected fragment on symbol data!");
> +  SD.setFragment(F);
> +  SD.setOffset(F->getContents().size());
>  }
--

 - Daniel


On Fri, Jul 16, 2010 at 6:13 PM, Michael Spencer <bigcheesegs at gmail.com> wrote:
> On Fri, Jul 16, 2010 at 11:25 AM, Daniel Dunbar <daniel at zuster.org> wrote:
>> Hi Michael,
>>
>> Overall patch looks good. I do have a few comments below. My main
>> comment is please try to make the style match that used in the
>> MCMachOStreamer more closely. I intend to refactor more functionality
>> into the base MCObjectStreamer class, and having them use consistent
>> idioms makes this easier; specific instances are included in the
>> comments:
>
> I implemented the changes and merged some stuff into MCObjectStreamer
> along with marking all the functions that are basically the same.
>
> http://github.com/Bigcheese/llvm-mirror/commit/294dbdfcf2b00347edb5c04371c710c6dd264fbd
>
> - Michael Spencer
>