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:
--> diff --git a/lib/MC/WinCOFFStreamer.cpp b/lib/MC/WinCOFFStreamer.cpp
> index 1030cdb..079a813 100644
> --- a/lib/MC/WinCOFFStreamer.cpp
> +++ b/lib/MC/WinCOFFStreamer.cpp
> @@ -18,24 +18,41 @@
> #include "llvm/MC/MCSection.h"
> #include "llvm/MC/MCSymbol.h"
> #include "llvm/MC/MCExpr.h"
> +#include "llvm/MC/MCValue.h"
> +#include "llvm/MC/MCAssembler.h"
> +#include "llvm/MC/MCAsmLayout.h"
> #include "llvm/MC/MCCodeEmitter.h"
> #include "llvm/MC/MCSectionCOFF.h"
> +#include "llvm/Target/TargetRegistry.h"
> #include "llvm/Target/TargetAsmBackend.h"
> +#include "llvm/ADT/StringMap.h"
> +
> #include "llvm/Support/COFF.h"
> #include "llvm/Support/Debug.h"
> #include "llvm/Support/ErrorHandling.h"
> #include "llvm/Support/raw_ostream.h"
> using namespace llvm;
>
> -#define dbg_notimpl(x) \
> - do { dbgs() << "not implemented, " << __FUNCTION__
<< " (" << x << ")"; \
> +#define dbgout(a, x) \
> + do { dbgs() << a << ", " << __FUNCTION__
<< " (" << x << ")"; \
> abort(); } while (false);
>
> +#define dbg_unexpected(x) dbgout("unexpected", x)
> +
> +#define dbg_notimpl(x) dbgout("not implemented", x)
> +
Please use llvm::report_fatal_error() for both of these instead of custom
macros.
> namespace {
> class WinCOFFStreamer : public MCObjectStreamer {
> public:
> + MCSymbol const *CurSymbol;
> +
> WinCOFFStreamer(MCContext &Context,
> TargetAsmBackend &TAB,
> MCCodeEmitter &CE,
> raw_ostream &OS);
>
> + void OnSectionDataCreated(MCSection const *Section,
> + MCSectionData *SectionData);
> +
> + void OnSymbolDataCreated(MCSymbol const *Symbol,
> + MCSymbolData *SymbolData);
These aren't used, can you just remove them and add them when they actually
are
needed?
> + MCSectionData *getSectionData(MCSection const *Section = 0);
> + MCSymbolData *getSymbolData(MCSymbol const *Symbol);
> +
> + void AddFragment(MCFragment *Fragment);
This function isn't necessary, fragments see below.
> + void VisitSymbols(MCExpr const *Expr);
> +
> + void AddCommonSymbol(MCSymbol *Symbol, uint64_t Size,
> + unsigned ByteAlignment, bool External);
> +
> // MCStreamer interface
>
> virtual void EmitLabel(MCSymbol *Symbol);
> @@ -52,18 +79,18 @@ public:
> virtual void EndCOFFSymbolDef();
> virtual void EmitELFSize(MCSymbol *Symbol, const MCExpr *Value);
> virtual void EmitCommonSymbol(MCSymbol *Symbol, uint64_t Size,
> - unsigned ByteAlignment);
> + unsigned ByteAlignment);
> virtual void EmitLocalCommonSymbol(MCSymbol *Symbol, uint64_t Size);
> virtual void EmitZerofill(const MCSection *Section, MCSymbol *Symbol,
> - unsigned Size,unsigned ByteAlignment);
> + unsigned Size,unsigned ByteAlignment);
> virtual void EmitTBSSSymbol(const MCSection *Section, MCSymbol *Symbol,
> - uint64_t Size, unsigned ByteAlignment);
> + uint64_t Size, unsigned ByteAlignment);
> virtual void EmitBytes(StringRef Data, unsigned AddrSpace);
> virtual void EmitValue(const MCExpr *Value, unsigned Size,
> unsigned AddrSpace);
> virtual void EmitGPRel32Value(const MCExpr *Value);
> virtual void EmitValueToAlignment(unsigned ByteAlignment, int64_t Value,
> - unsigned ValueSize, unsigned MaxBytesToEmit);
> + unsigned ValueSize, unsigned
MaxBytesToEmit);
> virtual void EmitCodeAlignment(unsigned ByteAlignment,
> unsigned MaxBytesToEmit);
> virtual void EmitValueToOffset(const MCExpr *Offset, unsigned char
Value);
> @@ -79,11 +106,143 @@ WinCOFFStreamer::WinCOFFStreamer(MCContext
&Context,
> MCCodeEmitter &CE,
> raw_ostream &OS)
> : MCObjectStreamer(Context, TAB, OS, &CE) {
> + CurSymbol = NULL;
> + CurSection = NULL;
> +}
> +
> +
> +void WinCOFFStreamer::OnSectionDataCreated(MCSection const *Section,
> + MCSectionData *SectionData) {
> +}
> +
> +void WinCOFFStreamer::OnSymbolDataCreated(MCSymbol const *Symbol,
> + MCSymbolData *SymbolData) {
> +}
> +
> +MCSectionData *WinCOFFStreamer::getSectionData(MCSection const *Section) {
This function has two very different behaviors, one is to create the section
data for a particular section, the other is to get the section data for the
current section. Please remove the second use (assert Section != 0 if you like),
and use MCObjectStreamer::getCurrentSectionData() for that instead.
> + bool SectionDataCreated;
> +
> + assert((Section != 0) || (CurSection != 0) && "Must have a
valid section!");
> +
> + MCSectionData *SectionData = &getAssembler().getOrCreateSectionData(
> + *(Section ? Section : CurSection),
> + &SectionDataCreated
> + );
Please put the ); on the previous line (and in rest of file).
> +
> + if (SectionDataCreated)
> + OnSectionDataCreated(Section, SectionData);
> +
> + return SectionData;
> +}
> +
> +MCSymbolData *WinCOFFStreamer::getSymbolData(MCSymbol const *Symbol) {
> + bool SymbolDataCreated;
> +
> + MCSymbolData *SymbolData = &getAssembler().getOrCreateSymbolData(
> + *Symbol, &SymbolDataCreated
> + );
> +
> + if (SymbolDataCreated)
> + OnSymbolDataCreated(Symbol, SymbolData);
> +
> + return SymbolData;
> +}
> +
> +void WinCOFFStreamer::AddFragment(MCFragment *Fragment) {
> + MCSectionData *SectionData = getSectionData();
> +
> + SectionData->getFragmentList().addNodeToList(Fragment);
> +}
> +
> +void WinCOFFStreamer::VisitSymbols(MCExpr const *Expr) {
This function is equivalent to MCMachOStreamer::AddValueSymbols; please just
copy that version with a FIXME to refactor it into the base class (or go ahead
and do the refactoring).
> + switch (Expr->getKind()) {
> + case MCExpr::Target:
> + dbg_unexpected("unsupported expression type '" <<
*Expr << "'");
> + break;
> +
> + case MCExpr::Constant:
> + break;
> +
> + case MCExpr::Binary:
> + {
> + MCBinaryExpr const *BinaryExpr = cast<MCBinaryExpr>(Expr);
> +
> + VisitSymbols(BinaryExpr->getLHS());
> + VisitSymbols(BinaryExpr->getRHS());
> + }
> + break;
Please use:
--
case A: {
break;
}
--
style instead.
> +
> + case MCExpr::SymbolRef:
> + {
> + MCSymbolRefExpr const *SymbolRefExpr =
cast<MCSymbolRefExpr>(Expr);
> +
> + getSymbolData(&SymbolRefExpr->getSymbol());
> + }
> + break;
> +
> + case MCExpr::Unary:
> + {
> + MCUnaryExpr const *UnaryExpr = cast<MCUnaryExpr>(Expr);
> +
> + VisitSymbols(UnaryExpr->getSubExpr());
> + }
> + break;
> + }
> +}
> +
> +void WinCOFFStreamer::AddCommonSymbol(MCSymbol *Symbol, uint64_t Size,
> + unsigned ByteAlignment, bool
External) {
> + assert(!Symbol->isInSection() && "Symbol must not
already have a section!");
> +
> + std::string SectionName(".bss$linkonce");
> + SectionName.append(Symbol->getName().begin(),
Symbol->getName().end());
> +
> + MCSymbolData *SymbolData = getSymbolData(Symbol);
> +
> + unsigned Characteristics > + COFF::IMAGE_SCN_LNK_COMDAT |
> + COFF::IMAGE_SCN_CNT_UNINITIALIZED_DATA |
> + COFF::IMAGE_SCN_MEM_READ |
> + COFF::IMAGE_SCN_MEM_WRITE;
> +
> + int Selection = COFF::IMAGE_COMDAT_SELECT_LARGEST;
> +
> + MCSection const *Section = MCStreamer::getContext().getCOFFSection(
> + SectionName, Characteristics, Selection, SectionKind::getBSS()
> + );
Please spell the type as "const MCSection *". I would also eliminate
the
Selection variable.
> +
> + MCSectionData *SectionData = getSectionData(Section);
> +
> + if (SectionData->getAlignment() < ByteAlignment)
> + SectionData->setAlignment(ByteAlignment);
> +
> + SymbolData->setExternal(External);
> +
> + Symbol->setSection(*Section);
> +
> + if (ByteAlignment != 1)
> + AddFragment(
> + // FIXME: Who owns this memory?
> + new MCAlignFragment(ByteAlignment, 0, 0, ByteAlignment, SectionData)
> + );
This code is actually adding the fragment twice. The fragment implicitly adds
itself to the section if given, and the section takes ownership of it (this is
an idiom we use pervasively for LLVM IR, its very succinct/useful once you get
used to it). AddFragment() should be eliminated here and throughout the file.
> +
> + // FIXME: Who owns this memory?
> + SymbolData->Fragment = new MCFillFragment(
> + 0, 0, Size, SectionData
> + );
> +
> + AddFragment(SymbolData->Fragment);
> }
>
> // MCStreamer interface
>
> void WinCOFFStreamer::EmitLabel(MCSymbol *Symbol) {
> + // FIXME: Who owns this memory?
> + MCDataFragment *Fragment = new MCDataFragment(getSectionData());
There is no need to always create a new data fragment here; it is better to try
and reuse the current fragment if possible. Take a look at
MCMachOStreamer::EmitLabel for how I am doing this currently; I intend on moving
some of the helper functions like getOrCreateDataFragment to the base class.
> +
> + getSymbolData(Symbol)->Fragment = Fragment;
> +
> + AddFragment(Fragment);
> }
>
> void WinCOFFStreamer::EmitAssemblerFlag(MCAssemblerFlag Flag) {
> @@ -91,10 +250,29 @@ void
WinCOFFStreamer::EmitAssemblerFlag(MCAssemblerFlag Flag) {
> }
>
> void WinCOFFStreamer::EmitAssignment(MCSymbol *Symbol, const MCExpr
*Value) {
> + getSymbolData(Symbol);
> + VisitSymbols(Value);
> + Symbol->setVariableValue(Value);
> }
>
> void WinCOFFStreamer::EmitSymbolAttribute(MCSymbol *Symbol,
> MCSymbolAttr Attribute) {
> + switch (Attribute) {
> + case MCSA_WeakReference:
> + getSymbolData(Symbol)->modifyFlags(
> + COFF::SF_WeakReference,
> + COFF::SF_WeakReference
> + );
> + break;
> +
> + case MCSA_Global:
> + getSymbolData(Symbol)->setExternal(true);
> + break;
> +
> + default:
> + dbg_unexpected("unsupported attribute " << Attribute);
> + break;
> + }
> }
>
> void WinCOFFStreamer::EmitSymbolDesc(MCSymbol *Symbol, unsigned DescValue)
{
> @@ -102,15 +280,34 @@ void WinCOFFStreamer::EmitSymbolDesc(MCSymbol
*Symbol, unsigned DescValue) {
> }
>
> void WinCOFFStreamer::BeginCOFFSymbolDef(MCSymbol const *Symbol) {
> + assert(CurSymbol == NULL);
Please style asserts as: assert(Condition && "Explanation of the
assert"). It's
trivial in this case, but we use that style pervasively.
> + CurSymbol = Symbol;
> }
>
> void WinCOFFStreamer::EmitCOFFSymbolStorageClass(int StorageClass) {
> + assert(CurSymbol != NULL);
> + assert((StorageClass & ~0xFF) == 0);
> +
> + getSymbolData(CurSymbol)->modifyFlags(
> + StorageClass << COFF::SF_ClassShift,
> + COFF::SF_ClassMask
> + );
> }
>
> void WinCOFFStreamer::EmitCOFFSymbolType(int Type) {
> + assert(CurSymbol != NULL && "Must have a symbol to set its
type!");
> + assert((Type & ~0xFFFF) == 0 && "Type must only have
data in the first 2 "
> + "bytes");
> +
> + getSymbolData(CurSymbol)->modifyFlags(
> + Type << COFF::SF_TypeShift,
> + COFF::SF_TypeMask
> + );
> }
>
> void WinCOFFStreamer::EndCOFFSymbolDef() {
> + assert(CurSymbol != NULL && "Must have a symbol to end
it!");
> + CurSymbol = NULL;
> }
>
> void WinCOFFStreamer::EmitELFSize(MCSymbol *Symbol, const MCExpr *Value) {
> @@ -118,14 +315,16 @@ void WinCOFFStreamer::EmitELFSize(MCSymbol *Symbol,
const MCExpr *Value) {
> }
>
> void WinCOFFStreamer::EmitCommonSymbol(MCSymbol *Symbol, uint64_t Size,
> - unsigned ByteAlignment) {
> + unsigned ByteAlignment) {
> + AddCommonSymbol(Symbol, Size, ByteAlignment, true);
> }
>
> void WinCOFFStreamer::EmitLocalCommonSymbol(MCSymbol *Symbol, uint64_t
Size) {
> + AddCommonSymbol(Symbol, Size, 1, false);
> }
>
> void WinCOFFStreamer::EmitZerofill(const MCSection *Section, MCSymbol
*Symbol,
> - unsigned Size,unsigned ByteAlignment) {
> + unsigned Size,unsigned ByteAlignment) {
> MCSectionCOFF const *SectionCOFF > static_cast<MCSectionCOFF
const *>(Section);
>
> @@ -145,10 +344,35 @@ void WinCOFFStreamer::EmitTBSSSymbol(const MCSection
*Section, MCSymbol *Symbol,
> }
>
> void WinCOFFStreamer::EmitBytes(StringRef Data, unsigned AddrSpace) {
> + MCDataFragment *Fragment = new MCDataFragment(getSectionData());
> +
This really shouldn't be creating new fragments for each call, see the
MCMachOStreamer implementation.
> + Fragment->getContents().append(Data.begin(), Data.end());
> +
> + AddFragment(Fragment);
> }
>
> void WinCOFFStreamer::EmitValue(const MCExpr *Value, unsigned Size,
> - unsigned AddrSpace) {
> + unsigned AddrSpace) {
> + VisitSymbols(Value);
> +
> + MCDataFragment *Fragment = new MCDataFragment(getSectionData());
> +
> + assert(AddrSpace == 0);
> +
> + int64_t RawValue;
> +
> + if (Value->EvaluateAsAbsolute(RawValue)) {
> + char const *pvalue = reinterpret_cast<char const
*>(&RawValue);
> + Fragment->getContents().append(pvalue, pvalue + Size);
This isn't valid, it is not endian safe. You can copy the
MCMachOStreamer::EmitValue implementation (which is endian safe, but assumes the
target is little endian).
> + }
> + else
> + {
Please put braces on same line as else.
> + Fragment->getContents().resize(Size);
> + Fragment->addFixup(MCFixup::Create(0, Value,
> + MCFixup::getKindForSize(Size)));
> + }
> +
> + AddFragment(Fragment);
> }
>
> void WinCOFFStreamer::EmitGPRel32Value(const MCExpr *Value) {
> @@ -156,17 +380,33 @@ void WinCOFFStreamer::EmitGPRel32Value(const MCExpr
*Value) {
> }
>
> void WinCOFFStreamer::EmitValueToAlignment(unsigned ByteAlignment,
> - int64_t Value,
> - unsigned ValueSize,
> - unsigned MaxBytesToEmit) {
> + int64_t Value,
> + unsigned ValueSize,
> + unsigned MaxBytesToEmit) {
Currently you need this code from MCMachOStreamer, the contract on
MaxBytesToEmit differs between the parser and the backend.
--
if (MaxBytesToEmit == 0)
MaxBytesToEmit = ByteAlignment;
--
> + if (getSectionData()->getAlignment() < ByteAlignment)
> + getSectionData()->setAlignment(ByteAlignment);
> +
> + AddFragment(
> + new MCAlignFragment(
> + ByteAlignment, Value, ValueSize, MaxBytesToEmit, getSectionData()
> + )
> + );
> }
>
> void WinCOFFStreamer::EmitCodeAlignment(unsigned ByteAlignment,
> - unsigned MaxBytesToEmit = 0) {
> + unsigned MaxBytesToEmit = 0) {
Same comment as above w.r.t. MaxBytesToEmit. Also, the default argument here is
invalid and should be removed.
> + if (getSectionData()->getAlignment() < ByteAlignment)
> + getSectionData()->setAlignment(ByteAlignment);
> +
> + MCAlignFragment *Fragment = new llvm::MCAlignFragment(
> + ByteAlignment, 0, 1, MaxBytesToEmit, getSectionData()
> + );
> + Fragment->setEmitNops(true);
> + AddFragment(Fragment);
> }
>
> void WinCOFFStreamer::EmitValueToOffset(const MCExpr *Offset,
> - unsigned char Value = 0) {
> + unsigned char Value = 0) {
The default argument here is invalid and should be removed.
> dbg_notimpl("Offset = '" << *Offset <<
"', Value = " << Value);
> }
>
> @@ -176,11 +416,30 @@ void WinCOFFStreamer::EmitFileDirective(StringRef
Filename) {
> }
>
> void WinCOFFStreamer::EmitDwarfFileDirective(unsigned FileNo,
> - StringRef Filename) {
> + StringRef Filename) {
> dbg_notimpl("FileNo = " << FileNo << ",
Filename = '" << Filename << "'");
> }
>
> void WinCOFFStreamer::EmitInstruction(const MCInst &Instruction) {
> + for (unsigned i = 0, e = Instruction.getNumOperands(); i != e; ++i)
> + if (Instruction.getOperand(i).isExpr())
> + VisitSymbols(Instruction.getOperand(i).getExpr());
> +
> + getSectionData()->setHasInstructions(true);
> +
> + // FIXME: Who owns this memory?
> + MCInstFragment *Fragment > + new MCInstFragment(Instruction,
getSectionData());
> +
> + {
> + raw_svector_ostream VecOS(Fragment->getCode());
> +
> + getAssembler().getEmitter().EncodeInstruction(Instruction, VecOS,
Fragment->getFixups());
> +
> + // VecOS's destructor calls flush.
> + }
The nested block here is unnecessary.
> +
> + AddFragment(Fragment);
> }
>
> void WinCOFFStreamer::Finish() {
--
Thanks again for your work, I'm please to see it coming along!
- Daniel
On Wed, Jul 14, 2010 at 1:54 AM, Michael Spencer <bigcheesegs at
gmail.com> wrote:> On Sun, Jul 11, 2010 at 6:10 PM, Chris Lattner <clattner at
apple.com> wrote:
>> This probably needs to be slightly tweaked to work with mainline. I
don't see anything objectionable, but I think Daniel needs to review this
one.
>
> Updated patch to work with mainline.
>
>
http://github.com/Bigcheese/llvm-mirror/commit/d19a4c82c18afc4830c09b70f02d162292231c94
>
> - Michael Spencer
>