Provide the name for labels inside MS asm blocks to clang as the branch targets for each asm block

[• ehsan]

Summary

These branch targets will help clang diagnose jumps across inline MS asm blocks.

[rnk]

I don't think we need this, LookupInlineAsmLabel should be able to track all the LabelDecl's that got created during inline asm parsing.

[• ehsan]

In D5515#4, @rnk wrote:

I don't think we need this, LookupInlineAsmLabel should be able to track all the LabelDecl's that got created during inline asm parsing.

Well, LookupInlineAsmLabel would not be able to associate a label name with an MSAsmStmt, which we later on need to use in order to check whether the branch target is defined inside another MSAsmStmt on the clang side. Also, note that this patch provides two bits of information, the "labels defined" and the "branch targets". The branch targets cannot be deciphered from LookupInlineAsmLabel, given the fact that you can do things such as mov eax, LabelName that are not branches.

[rnk]

In D5515#5, @ehsan wrote:

Well, LookupInlineAsmLabel would not be able to associate a label name with an MSAsmStmt, which we later on need to use in order to check whether the branch target is defined inside another MSAsmStmt on the clang side. Also, note that this patch provides two bits of information, the "labels defined" and the "branch targets". The branch targets cannot be deciphered from LookupInlineAsmLabel, given the fact that you can do things such as mov eax, LabelName that are not branches.

I was imagining having ClangAsmParserCallback track a list of defined and used LabelDecls. The 'Create' parameter basically means "this label was defined" and could be renamed to reflect that.

I guess the one thing that is completely lost is the branch information. You only know what uses there are, not if the use is a branch. Is that OK? It means we need to reject this code:

void g();
void *f() {
  __asm foo: nop
  g();
  __asm mov eax, foo
}

I guess I'm OK with that. :)

[• ehsan]

In D5515#6, @rnk wrote:

In D5515#5, @ehsan wrote:

Well, LookupInlineAsmLabel would not be able to associate a label name with an MSAsmStmt, which we later on need to use in order to check whether the branch target is defined inside another MSAsmStmt on the clang side. Also, note that this patch provides two bits of information, the "labels defined" and the "branch targets". The branch targets cannot be deciphered from LookupInlineAsmLabel, given the fact that you can do things such as mov eax, LabelName that are not branches.

I was imagining having ClangAsmParserCallback track a list of defined and used LabelDecls. The 'Create' parameter basically means "this label was defined" and could be renamed to reflect that.

But ClangAsmParserCallback right now doesn't know whether the create vs use happens within the same asm block... And it is invoked before we even create the corresponding MSAsmStmt, so I'm not sure where it would record this information. Any pointers?

I guess the one thing that is completely lost is the branch information. You only know what uses there are, not if the use is a branch. Is that OK? It means we need to reject this code:

void g();
void *f() {
  __asm foo: nop
  g();
  __asm mov eax, foo
}

I guess I'm OK with that. :)

That would be a bit unfortunate, since my current patch properly accepts that code. But I guess I don't feel very strongly.

[• ehsan]

Abandoned in favor of http://reviews.llvm.org/D5694.

include/llvm/MC/MCParser/MCAsmParser.h

public:
   virtual bool isParsingInlineAsm() = 0;
 
   /// parseMSInlineAsm - Parse ms-style inline assembly.
   virtual bool parseMSInlineAsm(void *AsmLoc, std::string &AsmString,
                                 unsigned &NumOutputs, unsigned &NumInputs,
                                 SmallVectorImpl<std::pair<void *, bool> > &OpDecls,
                                 SmallVectorImpl<std::string> &Constraints,
                                 SmallVectorImpl<std::string> &Clobbers,
+                                SmallVectorImpl<std::string> &BranchTargets,
+                                SmallVectorImpl<std::string> &LabelsDefined,
                                 const MCInstrInfo *MII,
                                 const MCInstPrinter *IP,
                                 MCAsmParserSemaCallback &SI) = 0;
 
   /// Note - Emit a note at the location \p L, with the message \p Msg.
   virtual void Note(SMLoc L, const Twine &Msg,
                     ArrayRef<SMRange> Ranges = None) = 0;
 

lib/MC/MCParser/AsmParser.cpp

 public:
   MacroInstantiation(SMLoc IL, int EB, SMLoc EL, size_t CondStackDepth);
 };
 
 struct ParseStatementInfo {
   /// \brief The parsed operands from the last parsed statement.
   SmallVector<std::unique_ptr<MCParsedAsmOperand>, 8> ParsedOperands;
 
+  /// \brief The name of a label, in case one was encountered.
+  StringRef LabelName;
+
   /// \brief The opcode from the last parsed instruction.
   unsigned Opcode;
 
   /// \brief Was there an error parsing the inline assembly?
   bool ParseError;
 
   SmallVectorImpl<AsmRewrite> *AsmRewrites;
 
public:
   void setParsingInlineAsm(bool V) override { ParsingInlineAsm = V; }
   bool isParsingInlineAsm() override { return ParsingInlineAsm; }
 
   bool parseMSInlineAsm(void *AsmLoc, std::string &AsmString,
                         unsigned &NumOutputs, unsigned &NumInputs,
                         SmallVectorImpl<std::pair<void *,bool> > &OpDecls,
                         SmallVectorImpl<std::string> &Constraints,
                         SmallVectorImpl<std::string> &Clobbers,
+                        SmallVectorImpl<std::string> &BranchTargets,
+                        SmallVectorImpl<std::string> &LabelsDefined,
                         const MCInstrInfo *MII, const MCInstPrinter *IP,
                         MCAsmParserSemaCallback &SI) override;
 
   bool parseExpression(const MCExpr *&Res);
   bool parseExpression(const MCExpr *&Res, SMLoc &EndLoc) override;
   bool parsePrimaryExpr(const MCExpr *&Res, SMLoc &EndLoc) override;
   bool parseParenExpression(const MCExpr *&Res, SMLoc &EndLoc) override;
   bool parseAbsoluteExpression(int64_t &Res) override;
case AsmToken::Colon: {
     // implicitly marked as external.
     MCSymbol *Sym;
     if (LocalLabelVal == -1) {
       if (ParsingInlineAsm && SI) {
         StringRef RewrittenLabel = SI->LookupInlineAsmLabel(IDVal, getSourceManager(), IDLoc, true);
         assert(RewrittenLabel.size() && "We should have an internal name here.");
         Info.AsmRewrites->push_back(AsmRewrite(AOK_Label, IDLoc,
                                                IDVal.size(), RewrittenLabel));
+        Info.LabelName = IDVal;
         IDVal = RewrittenLabel;
       }
       Sym = getContext().GetOrCreateSymbol(IDVal);
     } else
       Sym = Ctx.CreateDirectionalLocalSymbol(LocalLabelVal);
     if (!Sym->isUndefined() || Sym->isVariable())
       return Error(IDLoc, "invalid symbol redefinition");
 
if (AsmRewritePrecedence[AsmRewriteA->Kind] <
     return 1;
   llvm_unreachable("Unstable rewrite sort.");
 }
 
 bool AsmParser::parseMSInlineAsm(
     void *AsmLoc, std::string &AsmString, unsigned &NumOutputs,
     unsigned &NumInputs, SmallVectorImpl<std::pair<void *, bool> > &OpDecls,
     SmallVectorImpl<std::string> &Constraints,
-    SmallVectorImpl<std::string> &Clobbers, const MCInstrInfo *MII,
+    SmallVectorImpl<std::string> &Clobbers,
+    SmallVectorImpl<std::string> &BranchTargets,
+    SmallVectorImpl<std::string> &LabelsDefined,
+    const MCInstrInfo *MII,
     const MCInstPrinter *IP, MCAsmParserSemaCallback &SI) {
   SmallVector<void *, 4> InputDecls;
   SmallVector<void *, 4> OutputDecls;
   SmallVector<bool, 4> InputDeclsAddressOf;
   SmallVector<bool, 4> OutputDeclsAddressOf;
   SmallVector<std::string, 4> InputConstraints;
   SmallVector<std::string, 4> OutputConstraints;
   SmallVector<unsigned, 4> ClobberRegs;
bool AsmParser::parseMSInlineAsm(
   while (getLexer().isNot(AsmToken::Eof)) {
     ParseStatementInfo Info(&AsmStrRewrites);
     if (parseStatement(Info, &SI))
       return true;
 
     if (Info.ParseError)
       return true;
 
+    if (!Info.LabelName.empty())
+      LabelsDefined.push_back(Info.LabelName);
+
     if (Info.Opcode == ~0U)
       continue;
 
     const MCInstrDesc &Desc = MII->get(Info.Opcode);
 
     // Build the list of clobbers, outputs and inputs.
     for (unsigned i = 1, e = Info.ParsedOperands.size(); i != e; ++i) {
       MCParsedAsmOperand &Operand = *Info.ParsedOperands[i];
for (unsigned i = 1, e = Info.ParsedOperands.size(); i != e; ++i) {
         continue;
       }
 
       // Expr/Input or Output.
       StringRef SymName = Operand.getSymName();
       if (SymName.empty())
         continue;
 
+      // Build the list of branch targets.
+      if (Desc.isBranch() && Operand.isMem())
+        BranchTargets.push_back(SymName);
+
       void *OpDecl = Operand.getOpDecl();
       if (!OpDecl)
         continue;
 
       bool isOutput = (i == 1) && Desc.mayStore();
       SMLoc Start = SMLoc::getFromPointer(SymName.data());
       if (isOutput) {
         ++InputIdx;