Fix .thumb_func directive in ARMAsmParser

[enefaim]

Summary

This patch is intended to solve the following FIXME in ARMAsmParser::parseDirectiveThumbFunc:

// FIXME: assuming function name will be the line following .thumb_func
// We really should be checking the next symbol definition even if there's
// stuff in between.

I didn't add any new test just modified the existing one by moving the .type directive between the .thumb_func directive
and the actual symbol (you can check it in the diff). This is how GCC places the directives (at least it did it for me when
I compiled a simple "Hello world!" C program to assembly code).

[t.p.northover]

Hi Gabor,

Thanks for doing this. One tiny comment, but other than that it looks fine. Feel free to commit with that change.

Cheers.

Tim.

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

77

Very minor point: variable names should start with a capital letter in LLVM.

[t.p.northover]

Gabor sent me an updated patch, which I've committed as r193403.

[rengolin]

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

7848

onLabelParsed() is a generic function, and you're assuming that it'll always be used to emit thumb function symbols. If you want to make sure it's only used for that, either call it emitLazyThumbFunc() or something like that, or add a check/switch/assert to this function to make sure you only use it when intended.

7868

Now, both MachO and ELF look identical, and the code that deals with both also look to be doing the exact same thing, but in different ways.

I like the idea of a lazy ThumbFunc emitter, so you can personalize object / assembly files differently. Can you remove this part of the code and make sure your lazy function is called on all needs, assembly, ELF, MachO, etc?

[rengolin]

I only realized now that if I add comments inline I still *have* to press
"clowcopterize" (whatever that means).

GitHub is so much better than this...

cheers,
--renato

include/llvm/MC/MCTargetAsmParser.h

   virtual void convertToMapAndConstraints(unsigned Kind,
                       const SmallVectorImpl<MCParsedAsmOperand*> &Operands) = 0;
 
   virtual const MCExpr *applyModifierToExpr(const MCExpr *E,
                                             MCSymbolRefExpr::VariantKind,
                                             MCContext &Ctx) {
     return 0;
   }
+
+  virtual void onLabelParsed(MCSymbol *Symbol) { };
 };
 
 } // End llvm namespace
 
 #endif

lib/MC/MCParser/AsmParser.cpp

       Out.EmitLabel(Sym);
 
     // If we are generating dwarf for assembly source files then gather the
     // info to make a dwarf label entry for this label if needed.
     if (getContext().getGenDwarfForAssembly())
       MCGenDwarfLabelEntry::Make(Sym, &getStreamer(), getSourceManager(),
                                  IDLoc);
 
+    getTargetParser().onLabelParsed(Sym);
+
     // Consume any end of statement token, if present, to avoid spurious
     // AddBlankLine calls().
     if (Lexer.is(AsmToken::EndOfStatement)) {
       Lex();
       if (Lexer.is(AsmToken::Eof))
         return false;
     }
 

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

     PersonalityLoc = SMLoc();
     HandlerDataLoc = SMLoc();
     FPReg = -1;
   }
 
   // Map of register aliases registers via the .req directive.
   StringMap<unsigned> RegisterReqs;
 
+  bool nextSymbolIsThumb;
+
   struct {
     ARMCC::CondCodes Cond;    // Condition for IT block.
     unsigned Mask:4;          // Condition mask for instructions.
                               // Starting at first 1 (from lsb).
                               //   '1'  condition as indicated in IT.
                               //   '0'  inverse of condition (else).
                               // Count of instructions in IT block is
                               // 4 - trailingzeroes(mask)

     // Cache the MCRegisterInfo.
     MRI = getContext().getRegisterInfo();
 
     // Initialize the set of available features.
     setAvailableFeatures(ComputeAvailableFeatures(STI.getFeatureBits()));
 
     // Not in an ITBlock to start with.
     ITState.CurPosition = ~0U;
+
+    nextSymbolIsThumb = false;
   }
 
   // Implementation of the MCTargetAsmParser interface:
   bool ParseRegister(unsigned &RegNo, SMLoc &StartLoc, SMLoc &EndLoc);
   bool ParseInstruction(ParseInstructionInfo &Info, StringRef Name,
                         SMLoc NameLoc,
                         SmallVectorImpl<MCParsedAsmOperand*> &Operands);
   bool ParseDirective(AsmToken DirectiveID);
 
   unsigned validateTargetOperandClass(MCParsedAsmOperand *Op, unsigned Kind);
   unsigned checkTargetMatchPredicate(MCInst &Inst);
 
   bool MatchAndEmitInstruction(SMLoc IDLoc, unsigned &Opcode,
                                SmallVectorImpl<MCParsedAsmOperand*> &Operands,
                                MCStreamer &Out, unsigned &ErrorInfo,
                                bool MatchingInlineAsm);
+  void onLabelParsed(MCSymbol *Symbol);
+
 };
 } // end anonymous namespace
 
 namespace {
 
 /// ARMOperand - Instances of this class represent a parsed ARM machine
 /// operand.
 class ARMOperand : public MCParsedAsmOperand {

     return Error(L, "target does not support ARM mode");
 
   if (isThumb())
     SwitchMode();
   getParser().getStreamer().EmitAssemblerFlag(MCAF_Code32);
   return false;
 }
 
+void ARMAsmParser::onLabelParsed(MCSymbol *Symbol) {
+  if (nextSymbolIsThumb) {
+    getParser().getStreamer().EmitThumbFunc(Symbol);
+    nextSymbolIsThumb = false;
+  }
+}
+
 /// parseDirectiveThumbFunc
 ///  ::= .thumbfunc symbol_name
 bool ARMAsmParser::parseDirectiveThumbFunc(SMLoc L) {
   const MCAsmInfo *MAI = getParser().getStreamer().getContext().getAsmInfo();
   bool isMachO = MAI->hasSubsectionsViaSymbols();
-  StringRef Name;
-  bool needFuncName = true;
 
   // Darwin asm has (optionally) function name after .thumb_func direction
   // ELF doesn't
   if (isMachO) {
     const AsmToken &Tok = Parser.getTok();
     if (Tok.isNot(AsmToken::EndOfStatement)) {
       if (Tok.isNot(AsmToken::Identifier) && Tok.isNot(AsmToken::String))
         return Error(L, "unexpected token in .thumb_func directive");
-      Name = Tok.getIdentifier();
+      MCSymbol *Func =
+          getParser().getContext().GetOrCreateSymbol(Tok.getIdentifier());
+      getParser().getStreamer().EmitThumbFunc(Func);
       Parser.Lex(); // Consume the identifier token.
-      needFuncName = false;
+      return false;
     }
   }
 
   if (getLexer().isNot(AsmToken::EndOfStatement))
     return Error(L, "unexpected token in directive");
 
-  // Eat the end of statement and any blank lines that follow.
-  while (getLexer().is(AsmToken::EndOfStatement))
-    Parser.Lex();
-
-  // FIXME: assuming function name will be the line following .thumb_func
-  // We really should be checking the next symbol definition even if there's
-  // stuff in between.
-  if (needFuncName) {
-    Name = Parser.getTok().getIdentifier();
-  }
+  nextSymbolIsThumb = true;
 
-  // Mark symbol as a thumb symbol.
-  MCSymbol *Func = getParser().getContext().GetOrCreateSymbol(Name);
-  getParser().getStreamer().EmitThumbFunc(Func);
   return false;
 }
 
 /// parseDirectiveSyntax
 ///  ::= .syntax unified | divided
 bool ARMAsmParser::parseDirectiveSyntax(SMLoc L) {
   const AsmToken &Tok = Parser.getTok();
   if (Tok.isNot(AsmToken::Identifier))

test/MC/ARM/elf-thumbfunc.s

 @@ test st_value bit 0 of thumb function
 @ RUN: llvm-mc %s -triple=thumbv7-linux-gnueabi -filetype=obj -o - | \
 @ RUN: llvm-readobj -t | FileCheck %s
   .syntax unified
   .text
   .globl  foo
   .align  2
-  .type foo,%function
   .code 16
   .thumb_func
+  .type foo,%function
 foo:
   bx  lr
 
 @@ make sure foo is thumb function: bit 0 = 1 (st_value)
 @CHECK:        Symbol {
 @CHECK:          Name: foo
 @CHECK-NEXT:     Value: 0x1
 @CHECK-NEXT:     Size: 0
 @CHECK-NEXT:     Binding: Global
 @CHECK-NEXT:     Type: Function