[llvm-commits] [llvm] r79612 - in /llvm/trunk: include/llvm/MC/MCAssembler.h lib/MC/MCAssembler.cpp lib/MC/MCMachOStreamer.cpp test/MC/MachO/ test/MC/MachO/dg.exp test/MC/MachO/sections.s test/Scripts/macho-dump tools/llvm-mc/llvm-mc.cpp

[Chris Lattner]

On Aug 21, 2009, at 2:11 AM, Daniel Dunbar wrote:
> URL: http://llvm.org/viewvc/llvm-project?rev=79612&view=rev
> Log:
> llvm-mc: Start MCAssembler and MCMachOStreamer.

Very nice Daniel!  Some thoughts:

> +
> +class MCFragment : public ilist_node<MCFragment> {

I think the MCFragment and subclasses should move to MCFragment.h at  
some point.


> +// FIXME: Should this be a separate class, or just merged into  
> MCSection? Since
> +// we anticipate the fast path being through an MCAssembler, the  
> only reason to
> +// keep it out is for API abstraction.
> +class MCSectionData : public ilist_node<MCSectionData> {


For now, MCAssembler should have a  
DenseMap<MCSection*,MCSectionData*>.  Why do these need to live in an  
ilist?  Is that just how you're maintaining the ordering within the  
resultant object file?  That seems strange because I think the  
ordering of sections needs to be per-segment.  Does it make sense to  
have an explicit MCMachOSegment class or something?

> +public:
> +  // Only for use as sentinel.
> +  MCSectionData();
> +  MCSectionData(const MCSection &Section, MCAssembler *A = 0);
> +
> +  const FragmentListType &getFragmentList() const { return  
> Fragments; }
> +  FragmentListType &getFragmentList() { return Fragments; }


It would be nice to vend iterators (frag_begin etc) instead of the  
ilist directly.

> +
> +  const MCSection &getSection() const { return Section; }
> +
> +  unsigned getAlignment() const { return Alignment; }
> +  void setAlignment(unsigned Value) { Alignment = Value; }

I don't have a strong opinion about alignment of the section, but is  
this really something that gets queried multiple times, or does this  
just get queried when the file is written?

> +class MCAssembler {
> +public:
> +  typedef iplist<MCSectionData> SectionDataListType;
> +
> +  typedef SectionDataListType::const_iterator const_iterator;
> +  typedef SectionDataListType::iterator iterator;
> +
> +private:
> +  MCAssembler(const MCAssembler&);    // DO NOT IMPLEMENT
> +  void operator=(const MCAssembler&); // DO NOT IMPLEMENT
> +
> +  raw_ostream &OS;
> +
> +  iplist<MCSectionData> Sections;

Should this get declared with with type 'SectionDataListType'?

> +  const SectionDataListType &getSectionList() const { return  
> Sections; }
> +  SectionDataListType &getSectionList() { return Sections; }
> +
> +  iterator begin() { return Sections.begin(); }
> +  const_iterator begin() const { return Sections.begin(); }
> +
> +  iterator end() { return Sections.end(); }
> +  const_iterator end() const { return Sections.end(); }

How about section_ begin/end/iterator/size?  It is possible that  
MCAssembler will get other stuff to iterate over at some point and  
this makes it easier to read the client code.

> +++ llvm/trunk/lib/MC/MCAssembler.cpp Fri Aug 21 04:11:24 2009
> @@ -0,0 +1,228 @@
> +//===- lib/MC/MCAssembler.cpp - Assembler Backend Implementation  
> ----------===//
> +//
> +//                     The LLVM Compiler Infrastructure
> +//
> +// This file is distributed under the University of Illinois Open  
> Source
> +// License. See LICENSE.TXT for details.
> +//
> +// 
> = 
> = 
> = 
> ----------------------------------------------------------------------= 
> ==//
> +
> +#include "llvm/MC/MCAssembler.h"
> +#include "llvm/MC/MCSectionMachO.h"
> +#include "llvm/Support/DataTypes.h"
> +#include "llvm/Support/raw_ostream.h"
> +#include "llvm/Target/TargetMachOWriterInfo.h"
> +
> +using namespace llvm;
> +
> +namespace {
> +
> +class MachObjectWriter {
> +  // See <mach-o/loader.h>.
> +  enum {
> +    Header_Magic32 = 0xFEEDFACE,
> +    Header_Magic64 = 0xFEEDFACF
> +  };

It doesn't really matter at this point, but it would be good to  
eventually move the random constants out to a MachO.h file file lib/ 
CodeGen/MachO.h.

> +public:
> +  MachObjectWriter(raw_ostream &_OS, bool _IsLSB = true)
> +    : OS(_OS), IsLSB(_IsLSB) {

What is LSB?  little endian?  LSB != little endian :)

Overall, very nice!

-Chris