[PATCH][AsmParser][MC] Allow asm parser to emit elf header flags

Jim Grosbach grosbach at apple.com
Mon Aug 26 14:08:04 PDT 2013


Hi Vladimir,

Detailed comments inline below.

Generally speaking, please run new code through clang-format. There are lots of small formatting problems here and there that tool will help clean up without having to fuss about the details.

I don’t have much to comment on regarding the Mips specific side of things. You guys know that stuff better than I do.


> Index: test/MC/Mips/mips_directives.s
> ===================================================================
> --- test/MC/Mips/mips_directives.s	(revision 188764)
> +++ test/MC/Mips/mips_directives.s	(working copy)
> @@ -3,7 +3,10 @@
>  # CHECK:  .text
>  # CHECK:  $BB0_2:
>  $BB0_2:
> -  .ent directives_test
> +    .ent directives_test
> +    .abicalls
> +    .option pic0
> +    .set mips16
>      .frame    $sp,0,$ra
>      .mask     0x00000000,0
>      .fmask    0x00000000,0
> Index: include/llvm/MC/MCTargetAsmParser.h
> ===================================================================
> --- include/llvm/MC/MCTargetAsmParser.h	(revision 188764)
> +++ include/llvm/MC/MCTargetAsmParser.h	(working copy)
> @@ -174,6 +174,10 @@
>  
>    virtual void convertToMapAndConstraints(unsigned Kind,
>                        const SmallVectorImpl<MCParsedAsmOperand*> &Operands) = 0;
> +
> +  /// setAssemblerDialect - Target specific cleanup such as Ehdr flag setting.
> +  /// Analogous to MipsAsmPrinter::EmitEndOfAsmFile().
> +  virtual void emitEndOfObjFile(MCStreamer &Out) { }

The comment doesn’t match the function.

Relatedly, please refer to http://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments for the latest style preferences for these comments. I wouldn’t trust that the surrounding code is up-to-date.

That said, why is this function necessary at all? EmitEndOfAsmFile() is called even for object files.

>  };
>  
>  } // End llvm namespace
> Index: lib/Target/Mips/AsmParser/MipsAsmParser.cpp
> ===================================================================
> --- lib/Target/Mips/AsmParser/MipsAsmParser.cpp	(revision 188764)
> +++ lib/Target/Mips/AsmParser/MipsAsmParser.cpp	(working copy)
> @@ -7,6 +7,8 @@
>  //
>  //===----------------------------------------------------------------------===//
>  
> +#include "AsmParser/MipsAsmFlags.h"
> +#include "MCTargetDesc/MipsELFStreamer.h"
>  #include "MCTargetDesc/MipsMCTargetDesc.h"
>  #include "MipsRegisterInfo.h"
>  #include "llvm/ADT/StringSwitch.h"
> @@ -63,6 +65,7 @@
>    MCSubtargetInfo &STI;
>    MCAsmParser &Parser;
>    MipsAssemblerOptions Options;
> +  MipsMCAsmFlags Flags;
>    bool hasConsumedDollar;
>  
>  #define GET_ASSEMBLER_HEADER
> @@ -166,6 +169,8 @@
>  
>    bool parseDirectiveWord(unsigned Size, SMLoc L);
>  
> +  bool parseDirectiveOption();
> +
>    MCSymbolRefExpr::VariantKind getVariantKind(StringRef Symbol);
>  
>    bool isMips64() const {
> @@ -198,11 +203,20 @@
>  
>    bool processInstruction(MCInst &Inst, SMLoc IDLoc,
>                          SmallVectorImpl<MCInst> &Instructions);
> +  void emitEndOfObjFile(MCStreamer &Out);
> +
> +  void setAbiTy();
> +  void setArchTy();
> +  void setAseTy();

setXXX() is the usual name for the setter of a getter/setter pair. These are a different sort of thing, and should be named differently. Comments about what they do would be good.


> +
>  public:
>    MipsAsmParser(MCSubtargetInfo &sti, MCAsmParser &parser)
>      : MCTargetAsmParser(), STI(sti), Parser(parser), hasConsumedDollar(false) {
>      // Initialize the set of available features.
>      setAvailableFeatures(ComputeAvailableFeatures(STI.getFeatureBits()));
> +    setAbiTy();
> +    setArchTy();
> +    setAseTy();

Since these are just called once, and all in order, why have three separate functions at all?

>    }
>  
>    MCAsmParser &getParser() const { return Parser; }
> @@ -446,6 +460,43 @@
>    return MipsInsts[Opcode];
>  }
>  
> +void MipsAsmParser::setAbiTy() {
> +  uint64_t FeatureBits = STI.getFeatureBits();
> +  unsigned AbiType = MipsMCAsmFlags::MAF_ABI_DEFAULT;
> +  if (FeatureBits & Mips::FeatureN32)
> +    AbiType = MipsMCAsmFlags::MAF_ABI_N32;
> +  else if (FeatureBits & Mips::FeatureO32)
> +    AbiType = MipsMCAsmFlags::MAF_ABI_O32;
> +  else if (FeatureBits & Mips::FeatureN64)
> +    AbiType = MipsMCAsmFlags::MAF_ABI_N64;
> +
> +  Flags.setAbi(AbiType);
> +}
> +void MipsAsmParser::setArchTy() {
> +    uint64_t FeatureBits = STI.getFeatureBits();
> +  unsigned ArchType = MipsMCAsmFlags::MAF_ARCH_DEFAULT;
> +  if (FeatureBits & Mips::FeatureMips64r2)
> +    ArchType = MipsMCAsmFlags::MAF_ARCH_64R2;
> +  else if (FeatureBits & Mips::FeatureMips64)
> +    ArchType = MipsMCAsmFlags::MAF_ARCH_64R1;
> +  else if (FeatureBits & Mips::FeatureMips32r2)
> +    ArchType = MipsMCAsmFlags::MAF_ARCH_32R2;
> +  else if (FeatureBits & Mips::FeatureMips32)
> +    ArchType = MipsMCAsmFlags::MAF_ARCH_32R1;
> +
> +  Flags.setArch(ArchType);
> +
> +}
> +void MipsAsmParser::setAseTy() {
> +  uint64_t FeatureBits = STI.getFeatureBits();
> +  unsigned AseType = MipsMCAsmFlags::MAF_ASE_DEFAULT;
> +  if (FeatureBits & Mips::FeatureMips16)
> +    AseType = MipsMCAsmFlags::MAF_ASE_MIPS16;
> +  else if (FeatureBits & Mips::FeatureMicroMips)
> +    AseType = MipsMCAsmFlags::MAF_ASE_MICROMIPS;
> +  Flags.setAse(AseType);
> +}
> +
>  bool MipsAsmParser::processInstruction(MCInst &Inst, SMLoc IDLoc,
>                                         SmallVectorImpl<MCInst> &Instructions) {
>    const MCInstrDesc &MCID = getInstDesc(Inst.getOpcode());
> @@ -1807,14 +1858,30 @@
>    } else if (Tok.getString() == "nomacro") {
>      return parseSetNoMacroDirective();
>    } else if (Tok.getString() == "nomips16") {
> -    // Ignore this directive for now.
> -    Parser.eatToEndOfStatement();
> +    if ((STI.getFeatureBits() & Mips::FeatureMips16))
> +      STI.ToggleFeature(Mips::FeatureMips16);
> +    Flags.setAse(MipsMCAsmFlags::MAF_ASE_DEFAULT);
> +    Parser.Lex();
>      return false;
> -  } else if (Tok.getString() == "nomicromips") {
> -    // Ignore this directive for now.
> -    Parser.eatToEndOfStatement();
> +  } else if (Tok.getString() == "mips16") {
> +    if (!(STI.getFeatureBits() & Mips::FeatureMips16))
> +      STI.ToggleFeature(Mips::FeatureMips16);
> +    Flags.setAse(MipsMCAsmFlags::MAF_ASE_MIPS16);
> +    Parser.Lex();
>      return false;
> -  } else {
> +  }else if (Tok.getString() == "nomicromips") {
> +    if (STI.getFeatureBits() & Mips::FeatureMicroMips)
> +      STI.ToggleFeature(Mips::FeatureMicroMips);
> +    Flags.setAse(MipsMCAsmFlags::MAF_ASE_DEFAULT);
> +    Parser.Lex();
> +    return false;
> +  } else if (Tok.getString() == "micromips") {
> +    if (!(STI.getFeatureBits() & Mips::FeatureMicroMips))
> +      STI.ToggleFeature(Mips::FeatureMicroMips);
> +    Flags.setAse(MipsMCAsmFlags::MAF_ASE_MICROMIPS);
> +    Parser.Lex();
> +    return false;
> +  }else {
>      // It is just an identifier, look for an assignment.
>      parseSetAssignment();
>      return false;
> @@ -1848,6 +1915,36 @@
>    return false;
>  }
>  
> +bool MipsAsmParser::parseDirectiveOption() {
> +  // Get the option token
> +  AsmToken Tok = Parser.getTok();
> +  // At the moment only identifiers are supported
> +  if (Tok.isNot(AsmToken::Identifier)) {
> +    // Clear line
> +    Parser.eatToEndOfStatement();
> +    return Error(Tok.getLoc(), "unexpected token in directive");
> +  }
> +  StringRef Option = Tok.getIdentifier();
> +  if (Option == "pic0") {
> +    Flags.setRelocationModel(MipsMCAsmFlags::MAF_RM_STATIC);
> +    Parser.Lex();
> +    if (Parser.getTok().isNot(AsmToken::EndOfStatement))
> +      return Error(Parser.getTok().getLoc(), "unexpected token in directive");
> +    return false;
> +  }
> +
> +   if (Option == "pic2") {
> +    Flags.setRelocationModel(MipsMCAsmFlags::MAF_RM_PIC);
> +    Parser.Lex();
> +    if (Parser.getTok().isNot(AsmToken::EndOfStatement))
> +      return Error(Parser.getTok().getLoc(), "unexpected token in directive");
> +    return false;
> +  }
> +  // Unknown option
> +   Parser.eatToEndOfStatement();
> +   return Warning(Parser.getTok().getLoc(), "unknown option in directive");
> +}
> +
>  bool MipsAsmParser::ParseDirective(AsmToken DirectiveID) {
>  
>    StringRef IDVal = DirectiveID.getString();
> @@ -1897,9 +1994,29 @@
>      return false;
>    }
>  
> +  if (IDVal == ".abicalls") {
> +    // TODO: Set the Reloc::PIC_
> +    Flags.setRelocationModel(MipsMCAsmFlags::MAF_RM_PIC);
> +    if (Parser.getTok().isNot(AsmToken::EndOfStatement))
> +      return Error(Parser.getTok().getLoc(), "unexpected token in directive");
> +    return false;
> +  }
> +  if (IDVal == ".option") {
> +    return parseDirectiveOption();
> +  }
> +
>    return true;
>  }
>  
> +/// setAssemblerDialect -
> +/// Analogous to MipsAsmPrinter::EmitEndOfAsmFile().

Incorrect comment again.
> +void MipsAsmParser::emitEndOfObjFile(MCStreamer &OutStreamer)
> +{
> +  if (MipsELFStreamer *MES = dyn_cast<MipsELFStreamer>(&OutStreamer))
> +    MES->emitELFHeaderFlagsAsm(Flags);
> +  MCTargetAsmParser::emitEndOfObjFile(OutStreamer);
> +}

This can definitely be done without this function being added. See how ARM handles the EHDR flags.

> +
>  extern "C" void LLVMInitializeMipsAsmParser() {
>    RegisterMCAsmParser<MipsAsmParser> X(TheMipsTarget);
>    RegisterMCAsmParser<MipsAsmParser> Y(TheMipselTarget);
> Index: lib/Target/Mips/AsmParser/MipsAsmFlags.h
> ===================================================================
> --- lib/Target/Mips/AsmParser/MipsAsmFlags.h	(revision 0)
> +++ lib/Target/Mips/AsmParser/MipsAsmFlags.h	(revision 0)
> @@ -0,0 +1,93 @@
> +//=== MipsMCAsmFlags.h - MipsMCAsmFlags --------------------------------===//
> +//
> +//                    The LLVM Compiler Infrastructure
> +//
> +// This file is distributed under the University of Illinois Open Source
> +// License. See LICENCE.TXT for details.
> +//
> +//===-------------------------------------------------------------------===//
> +#ifndef MIPSMCASMFLAGS_H_
> +#define MIPSMCASMFLAGS_H_
> +
> +namespace llvm {
> +class MipsMCAsmFlags;
> +
> +class MipsMCAsmFlags {
> +public:
> +  enum MAFAbiTy {
> +    MAF_ABI_DEFAULT,
> +    MAF_ABI_O32,
> +    MAF_ABI_N32,
> +    MAF_ABI_N64
> +  } MAFAbi;
> +
> +  // These act as bit flags because more that one can be
> +  // active at the same time, sometimes ;-)
> +  enum MAFRelocationModelTy {
> +    MAF_RM_DEFAULT = 0,
> +    MAF_RM_STATIC = 1,
> +    MAF_RM_CPIC = 2,
> +    MAF_RM_PIC = 4
> +  } MAFRelocationModel;
> +
> +  enum MAFArchTy {
> +    MAF_ARCH_DEFAULT,
> +    MAF_ARCH_32R1,
> +    MAF_ARCH_32R2,
> +    MAF_ARCH_64R1,
> +    MAF_ARCH_64R2
> +  } MAFArch;
> +
> +  enum MAFAseTy {
> +    MAF_ASE_DEFAULT,
> +    MAF_ASE_MIPS16,
> +    MAF_ASE_MICROMIPS
> +  } MAFAse;
> +

Shouldn’t these be part of the MIPS ELF object file information if they’re not already? They’re not really internal to the MIPS AsmParser, right?

> +public:
> +  MipsMCAsmFlags()
> +    : abi(MAF_ABI_DEFAULT),
> +      model(MAF_RM_DEFAULT),
> +      arch(MAF_ARCH_DEFAULT),
> +      ase(MAF_ASE_DEFAULT) {}
> +
> +  ~MipsMCAsmFlags() {}
> +
> +  // This is simplistic. Mips16 may need on off switches.
> +  void setAbi(unsigned Abi) {(abi = Abi);}
> +  void setRelocationModel(unsigned RM) {(model |= RM);}
> +  void setArch(unsigned Arch) {(arch = Arch);}
> +  void setAse(unsigned Ase) {(ase = Ase);}
> +
> +  bool isAbiO32() const {return (abi == MAF_ABI_O32);}
> +  bool isAbiN32() const {return (abi == MAF_ABI_N32);}
> +  bool isAbiN64() const {return (abi == MAF_ABI_N64);}
> +  bool isAbiDefault() const {return (abi == MAF_ABI_DEFAULT);}
> +
> +  bool isModelCpic() const {return (model & MAF_RM_CPIC) == MAF_RM_CPIC;}
> +  bool isModelPic() const {return (model & MAF_RM_PIC) == MAF_RM_PIC;}
> +  bool isModelStatic() const {return (model & MAF_RM_STATIC) == MAF_RM_STATIC;}
> +  bool isModelDefault() const {
> +    return (model & MAF_RM_DEFAULT) == MAF_RM_DEFAULT;
> +  }
> +
> +  bool isArch32r1() const {return (arch == MAF_ARCH_32R1);}
> +  bool isArch32r2() const {return (arch == MAF_ARCH_32R2);}
> +  bool isArch64r1() const {return (arch == MAF_ARCH_64R1);}
> +  bool isArch64r2() const {return (arch == MAF_ARCH_64R2);}
> +  bool isArchDefault() const {return (arch == MAF_ARCH_DEFAULT);}
> +
> +  bool isAseMips16() const {return (ase == MAF_ASE_MIPS16);}
> +  bool isAseMicroMips() const {return (ase == MAF_ASE_MICROMIPS);}
> +  bool isAseDefault() const {return (ase == MAF_ASE_DEFAULT);}
> +
> +private:
> +  unsigned abi;  // o32, n32, n64, etc.
> +  unsigned model; // pic, cpic, etc.
> +  unsigned arch; // 32, 32r2, 64,etc.
> +  unsigned ase;  // 16, MicroMips, etc.

Class member variables shouldn’t be all-lower-case. See http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

> +};
> +
> +}
> +
> +#endif /* MIPSMCASMFLAGS_H_ */
> Index: lib/Target/Mips/MCTargetDesc/MipsELFStreamer.cpp
> ===================================================================
> --- lib/Target/Mips/MCTargetDesc/MipsELFStreamer.cpp	(revision 188764)
> +++ lib/Target/Mips/MCTargetDesc/MipsELFStreamer.cpp	(working copy)
> @@ -6,6 +6,7 @@
>  // License. See LICENSE.TXT for details.
>  //
>  //===-------------------------------------------------------------------===//
> +#include "AsmParser/MipsAsmFlags.h"
>  #include "MCTargetDesc/MipsELFStreamer.h"
>  #include "MipsSubtarget.h"
>  #include "llvm/MC/MCAssembler.h"
> @@ -74,6 +75,59 @@
>      MCA.setELFHeaderEFlags(EFlags);
>    }
>  
> +  // For llvm-mc. Set a group of ELF header flags
> +  void
> +  MipsELFStreamer::emitELFHeaderFlagsAsm(const MipsMCAsmFlags &Flags) {
> +
> +    // Update e_header flags
> +    MCAssembler& MCA = getAssembler();
> +    unsigned EFlags = MCA.getELFHeaderEFlags();
> +
> +    if (Flags.isAseMips16())
> +      EFlags |= ELF::EF_MIPS_ARCH_ASE_M16;
> +    else if (Flags.isAseMicroMips())
> +      EFlags |= ELF::EF_MIPS_ARCH_ASE_M16;
> +    else
> +      EFlags |= ELF::EF_MIPS_NOREORDER;
> +
> +    // Architecture
> +    if (Flags.isArch64r2())
> +      EFlags |= ELF::EF_MIPS_ARCH_64R2;
> +    else if (Flags.isArch64r1())
> +      EFlags |= ELF::EF_MIPS_ARCH_64;
> +    else if (Flags.isArch32r2())
> +      EFlags |= ELF::EF_MIPS_ARCH_32R2;
> +    else
> +      EFlags |= ELF::EF_MIPS_ARCH_32;
> +
> +    if (Flags.isAseMicroMips())
> +      EFlags |= ELF::EF_MIPS_MICROMIPS;
> +
> +    // ABI
> +    if (Flags.isAbiO32() || Flags.isAbiDefault())
> +      EFlags |= ELF::EF_MIPS_ABI_O32;
> +//    else if (Flags.isAbiN32())
> +//      EFlags |= ELF::EF_MIPS_ABI2;
> +//    else if (Flags.isAbiN64())
> +//      EFlags |= ELF::EF_MIPS_ABI2;

Please remove commented out code.

> +    else
> +      llvm_unreachable("Unsupported ABI for e_flags");
> +
> +    // Relocation Model
> +    // TODO: Need to add -mabicalls and -mno-abicalls flags.
> +    // Currently we assume that -mabicalls is the default.
> +    if (Flags.isModelCpic() || Flags.isModelDefault())
> +      EFlags |= ELF::EF_MIPS_CPIC;
> +    if (Flags.isModelPic() || Flags.isModelDefault())
> +      EFlags |= ELF::EF_MIPS_PIC;
> +    else if (Flags.isModelStatic())
> +      ; // Do nothing for Reloc::Static
> +    else
> +      llvm_unreachable("Unsupported relocation model for e_flags");
> +
> +    MCA.setELFHeaderEFlags(EFlags);
> +  }
> +
>    // For llc. Set a symbol's STO flags
>    void
>    MipsELFStreamer::emitMipsSTOCG(const MipsSubtarget &Subtarget,
> Index: lib/Target/Mips/MCTargetDesc/MipsELFStreamer.h
> ===================================================================
> --- lib/Target/Mips/MCTargetDesc/MipsELFStreamer.h	(revision 188764)
> +++ lib/Target/Mips/MCTargetDesc/MipsELFStreamer.h	(working copy)
> @@ -13,6 +13,7 @@
>  
>  namespace llvm {
>  class MipsAsmPrinter;
> +class MipsMCAsmFlags;
>  class MipsSubtarget;
>  class MCSymbol;
>  
> @@ -26,6 +27,7 @@
>  
>    ~MipsELFStreamer() {}
>    void emitELFHeaderFlagsCG(const MipsSubtarget &Subtarget);
> +  void emitELFHeaderFlagsAsm(const MipsMCAsmFlags &MAFlags);
>    void emitMipsSTOCG(const MipsSubtarget &Subtarget,
>                       MCSymbol *Sym,
>                       unsigned Val);

On Aug 20, 2013, at 7:28 AM, Vladimir Medic <Vladimir.Medic at imgtec.com> wrote:

> The assembler effects changes to the ELF environment through commandline
> options and inline directives. These need to be written out into the ELF
> header's eflags.
> 
> The mechanism for this is through the MCELFStreamer. We have modeled the
> assembler's version after the previously implemented codegen one.
> 
> I'm looking forward to your comments and remarks
> 
> Vladimir
> <MipsElfFlags.patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130826/261ee61e/attachment.html>


More information about the llvm-commits mailing list