[PATCH][AsmParser][MC] Allow asm parser to emit elf header flags
Jack Carter
Jack.Carter at imgtec.com
Thu Oct 3 13:46:58 PDT 2013
It seems so.
Since Vladimir is on Europe time I applied his patch, added the missing header, recompiled and tested and made a new patch which i have attached.
Jack
________________________________________
From: Rafael Espíndola [rafael.espindola at gmail.com]
Sent: Thursday, October 03, 2013 1:13 PM
To: Jack Carter
Cc: Vladimir Medic; llvm-commits at cs.uiuc.edu; Jim Grosbach; Rich Fuhler
Subject: Re: [PATCH][AsmParser][MC] Allow asm parser to emit elf header flags
The file AsmParser/MipsAsmFlags.h is missing from the patch.
On 2 October 2013 17:43, Jack Carter <Jack.Carter at imgtec.com> wrote:
> This time with the patch.
>
> Jack
> ________________________________
> From: Vladimir Medic
> Sent: Tuesday, September 10, 2013 12:52 AM
> To: llvm-commits at cs.uiuc.edu
> Cc: Jack Carter; Jim Grosbach
> Subject: RE: [PATCH][AsmParser][MC] Allow asm parser to emit elf header
> flags
>
> Hi all,
> This patch handles LLVM standalone assembler (llvm-mc) ELF flag setting
> based on input file
> directive processing.
>
> Mips assembly requires processing inline directives that directly and
> indirectly affect the output ELF header flags. This patch handles one
> ".abicalls".
>
> To process these directives we are following the model the code generator
> uses by storing state in a container as we go through processing and when
> we detect the end of input file processing, AsmParser is notified and we
> update the ELF header flags through a MipsELFStreamer method with a call
> from
> MCTargetAsmParser::emitEndOfAsmFile(MCStreamer &OutStreamer).
>
> It has been suggested that we use AsmPrinter::EmitEndOfAsmFile() to
> trigger the updating of the ELF header flags, but as far as we can tell
> MipsAsmPrinter is not available for the AsmParser.
>
> Also, the location of the call to emitEndOfAsmFile() is our best guess for
> when
> the input assembler file processing is at an end. If there is a better
> location
> for this please be specific and we will make the change.
>
> We have separate enum values for the relocation models than are in the ELF
> defines for Mips because static (non-shared) model is an active state, but
> is
> represented by 0 in the flags. Also we want to have a "default" state.
>
> Kind regards
>
> Vladimir
>
> ________________________________
> From: Jim Grosbach [grosbach at apple.com]
> Sent: Monday, August 26, 2013 11:08 PM
> To: Vladimir Medic
> Cc: llvm-commits at cs.uiuc.edu; Jack Carter
> Subject: Re: [PATCH][AsmParser][MC] Allow asm parser to emit elf header
> flags
>
> 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 --------------
A non-text attachment was scrubbed...
Name: elf_flags.patch
Type: text/x-patch
Size: 8706 bytes
Desc: elf_flags.patch
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131003/316f79ee/attachment.bin>
More information about the llvm-commits
mailing list