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

Jim Grosbach grosbach at apple.com
Thu Oct 3 16:13:02 PDT 2013


+  /// emitEndOfAsmFile -
+  /// End of assembly processing such as updating ELF header flags.
+  virtual void emitEndOfAsmFile(MCStreamer &Out) { }
+

+void MipsAsmParser::emitEndOfAsmFile(MCStreamer &OutStreamer)
+{
+  if (MipsELFStreamer *MES = dyn_cast<MipsELFStreamer>(&OutStreamer))

Braces don’t go on a new line. As Rafael suggested, please run this whole patch through clang-format. There are other formatting problems in this patch that the tool will help you clean up as well.

Please clean up this comment to describe the API and how it interacts with the general system. Referencing this specific case is interesting, but is not sufficient.

Other than that, Rafael’s already covered the rest of it.

Thanks for not giving up on this, guys.

-Jim

On Oct 3, 2013, at 3:45 PM, Jack Carter <Jack.Carter at imgtec.com> wrote:

> Ok, a more detailed response.
> ________________________________________
>> From: Rafael Espíndola [rafael.espindola at gmail.com]
>> Sent: Thursday, October 03, 2013 2:09 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
>> 
>> +  /// emitEndOfAsmFile -
>> 
>> Don't repeat the name of the asm function.
> 
> OK
> 
>> 
>> +  enum MAFRelocationModelTy
>> 
>> Don't you want this in a more visible place? For example, llvm-objdump
>> should be able to dump the flags in the ELF header, no?
> 
> No. Even though these look like ELF constructs, they are Mips assembler directives that do not automatically match directly. We are tracking the directives and will later may or may not translate them into one or more object format elements.
> 
>> 
>> It looks like only MAF_RM_DEFAULT and MAF_RM_CPIC are used. Why? Can
>> you just use a boolean?
> 
> Because these are the first of many. We cut down the patch to get folks to focus on the concept.
> 
>> 
>> +  MipsMCAsmFlags()
>> +    : Model(MAF_RM_DEFAULT){}
>> 
>> The formatting is wrong, please pass this via clang-format.
>> 
> 
> OK
> 
>> 
>> +/// emitEndOfAsmFile -
>> 
>> Don't duplicate the name.
>> 
> 
> OK
> 
>> 
>> +  if (MipsELFStreamer *MES = dyn_cast<MipsELFStreamer>(&OutStreamer))
>> 
>> Can't you use a cast? When would this be null?
>> 
> 
> We thought this was the preferred way. We can change it, but why?
> 
>> 
>> +#include "AsmParser/MipsAsmFlags.h"
>> #include "MCTargetDesc/MipsELFStreamer.h"
>> 
>> Wrong include order.
>> 
> 
> OK
> 
>>> 
>> +    else  if (RM != Reloc::Static) // Do nothing for Reloc::Static
>> +      llvm_unreachable("Unsupported relocation model for e_flags");
>> 
> 
> It can be an assert and we will change if, but why? Once again, we thought this was what we were suppose to do?
> 
>> This can be just an assert, no?
>> 
>> +  // For llvm-mc. Set a group of ELF header flags
>> 
>> This comment should be incorrect. Both llc and llvm-mc should use the
>> same code path.
>> 
> 
> This is what we really wanted a review for.
> 
> If this is not the correct way to implement this, Please let us know what is?
> 
> I implemented the Codegen method for this through the guidance of the list and am patterning the assembler's method after it. The Codegen stores the information about this information in a data structure that the assembler does not have access to. 
> 
> Remember, we have directives in the assembler code. We need to harvest it as we go along and at the end send information about it to the elf writer.
> 
> Any constructive guidance is appreciated. 
> Thanks,
> 
> Jack
> 
>> 
>> +#    .option pic0
>> +#    .set mips16
>> 
>> no commented code.
> 
> OK
> 
> 





More information about the llvm-commits mailing list