[PATCH][AsmParser][MC] Allow asm parser to emit elf header flags
Jim Grosbach
grosbach at apple.com
Tue Sep 17 09:47:47 PDT 2013
Not yet, sorry. I’m hoping to do so very soon. Thank you for the reminder.
-Jim
On Sep 17, 2013, at 5:19 AM, Vladimir Medic <Vladimir.Medic at imgtec.com> wrote:
> Hi all,
> did anyone get a chance to look at this?
>
> Thanks
>
> Vladimir
> From: Vladimir Medic
> Sent: Tuesday, September 10, 2013 9: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 --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130917/060f6931/attachment.html>
More information about the llvm-commits
mailing list