[llvm] Implement support for -defsym assembler option

[mgrang]

Summary

Changes to llvm-mc to move common logic to separate function.

Related clang patch: https://reviews.llvm.org/D26213

[colinl]

LGTM after changing to a forward declaration.

include/llvm/MC/MCContext.h

20

MCAsmParser should be forward declared instead of including the header.

[mgrang]

include/llvm/MC/MCContext.h

20

Forward declaration does not work in this case since the definition of MCAsmParser is required.

[mgrang]

Changed the type of "Value" in function setSymbolValue to int64_t.

[mgrang]

Replaced inclusion of MCAsmParser.h in MCContext.h with forward declaration of "class MCAsmParser".
Included MCAsmParser.h in MCContext.h instead.

[mgrang]

Minor fix:

• static int fillCommandLineSymbols(MCAsmParser &Parser){ + static int fillCommandLineSymbols(MCAsmParser &Parser) {

[mgrang]

Ping!

[mgrang]

Changed setSymbolValue to use Streamer directly instead of getting it from Parser.

[mgrang]

Reviewers,
Can we please get this and the related (https://reviews.llvm.org/D26213) patches reviewed.
We need these patches to support an internal customer.

Thanks,
Mandeep

[colinl]

lib/MC/MCContext.cpp

263

It's probably more idomatic to put this method on MCStreamer and pass in an MCContext reference.

[mgrang]

lib/MC/MCContext.cpp

263

I guess the setSymbolValue is better fit in MCContext since there are similar functions there
and all those functions are on MCContext.

[rengolin]

All the functionality tests need to be here. There are none.

In the Clang change, you need to make sure that the information is passed down, via target-features, -Wa and a dump of the values.

Here, you need to create tests that make use of those options (remember, clang is not always available when you build LLVM, so the test can't rely on that).

[mgrang]

@rengolin The -defsym option is already handled by llvm-mc (see http://llvm.org/viewvc/llvm-project?view=revision&revision=239240). This patch already added llvm-mc tests for defsym.

My patch makes the assembler (cc1as) recognize this option. Hence I put the assembly checks in the clang tests.

[mgrang]

Ping.

[rengolin]

Sorry, fell out of my radar. I really can't see how the tests would be moved in here.

LGTM.

[mgrang]

@rengolin @compnerd Thanks for the reviews!

[grosbach]

llvm/trunk/lib/MC/MCContext.cpp

263 ↗

(On Diff #79948)

The error checking shouldn't be in the setter function, but rather in the caller.

266 ↗

(On Diff #79948)

The MC layer should not be writing directly to errs(), but rather going through the diagnostics handlers.

[grosbach]

Please revert and fix.

[mgrang]

llvm/trunk/lib/MC/MCContext.cpp

263 ↗

(On Diff #79948)

@grosbach Thanks for your comments.

This function is called from llvm-mc as well as from cc1as_main in clang. Is it OK if I create an error checker function here and call it from both the call sites? Otherwise we would end up duplicating the error checking logic in both call sites.

266 ↗

(On Diff #79948)

@grosbach I will change this to go through diagnostics handlers.

However, I just moved this function from llvm-mc.cpp as is (almost). llvm-mc was already writing to errs(). Also I see places in MC where we directly write to errs(), like in MC/SubtargetFeature.cpp.

[echristo]

llvm/trunk/lib/MC/MCContext.cpp

263 ↗

(On Diff #79948)

The applications often have their own error handling. If you want to put something in Support that can be easily handled it's fine, but otherwise it's best if you just duplicate it. Happy to look at code that does either.

266 ↗

(On Diff #79948)

llvm-mc is an application and not a library. MC itself should not be writing to errs().

The uses in SubtargetFeature should be fixed, not promulgated further.

[grosbach]

llvm/trunk/lib/MC/MCContext.cpp

263 ↗

(On Diff #79948)

That's not the function of MCContext. The diagnostic checking really should be in the callers even if it's duplicated. Consider, for example, that llvm-mc is a development testing tool for LLVM developers and clang is an end-user tool. It is entirely possible that we would want different checking and behaviours for those use cases.

266 ↗

(On Diff #79948)

llvm-mc writing to errs() is fine. Stuff in lib/MC is not. The former is a development and testing tool. The latter is production library code.

The majority of the references to errs() in SubtargetFeature.cpp are also bugs and should be fixed.

[mgrang]

@grosbach @echristo Thanks for the explanations. I will push a fix.

[grosbach]

Thank you!

[t.p.northover]

llvm/trunk/lib/MC/MCContext.cpp

263 ↗

(On Diff #79948)

The diagnostic checking really should be in the callers even if it's duplicated

Really, the MC layer shouldn't even be in the business of parsing a user-facing Clang option like this (even if it's guaranteed error-free). Users should provide the symbol and the value separately.

[mgrang]

Pushed fixes:
https://reviews.llvm.org/D27359
https://reviews.llvm.org/D27360

[mgrang]

Patch no longer needed/valid.

[mgrang]

Patch no longer needed/valid.

include/llvm/MC/MCContext.h

 
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/MC/MCDwarf.h"
 #include "llvm/MC/MCSubtargetInfo.h"
 #include "llvm/MC/SectionKind.h"
 #include "llvm/Support/Allocator.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/raw_ostream.h"
 #include <map>
 #include <tuple>
 #include <vector> // FIXME: Shouldn't be needed.
 
public:
 
     MCSymbol *getOrCreateParentFrameOffsetSymbol(StringRef FuncName);
 
     MCSymbol *getOrCreateLSDASymbol(StringRef FuncName);
 
     /// Get the symbol for \p Name, or null.
     MCSymbol *lookupSymbol(const Twine &Name) const;
 
+    /// Set value for a symbol.
+    int setSymbolValue(MCStreamer &Streamer, std::string &I);
+
     /// getSymbols - Get a reference for the symbol table for clients that
     /// want to, for example, iterate over all symbols. 'const' because we
     /// still want any modifications to the table itself to use the MCContext
     /// APIs.
     const SymbolTable &getSymbols() const { return Symbols; }
 
     /// @}
 

lib/MC/MCContext.cpp

 }
 
 MCSymbol *MCContext::lookupSymbol(const Twine &Name) const {
   SmallString<128> NameSV;
   StringRef NameRef = Name.toStringRef(NameSV);
   return Symbols.lookup(NameRef);
 }
 
+int MCContext::setSymbolValue(MCStreamer &Streamer, std::string &I) {
+    auto Pair = StringRef(I).split('=');
+    if (Pair.second.empty()) {
+      errs() << "error: defsym must be of the form: sym=value: " << I << "\n";
+      return 1;
+    }
+    int64_t Value;
+    if (Pair.second.getAsInteger(0, Value)) {
+      errs() << "error: Value is not an integer: " << Pair.second << "\n";
+      return 1;
+    }
+    auto Symbol = getOrCreateSymbol(Pair.first);
+    Streamer.EmitAssignment(Symbol, MCConstantExpr::create(Value, *this));
+    return 0;
+}
+
 //===----------------------------------------------------------------------===//
 // Section Management
 //===----------------------------------------------------------------------===//
 
 MCSectionMachO *MCContext::getMachOSection(StringRef Segment, StringRef Section,
                                            unsigned TypeAndAttributes,
                                            unsigned Reserved2, SectionKind Kind,
                                            const char *BeginSymName) {

tools/llvm-mc/llvm-mc.cpp

while (Lexer.Lex().isNot(AsmToken::Eof)) {
     OS << " (\"";
     OS.write_escaped(Tok.getString());
     OS << "\")\n";
   }
 
   return Error;
 }
 
-static int fillCommandLineSymbols(MCAsmParser &Parser){
-  for(auto &I: DefineSymbol){
-    auto Pair = StringRef(I).split('=');
-    if(Pair.second.empty()){
-      errs() << "error: defsym must be of the form: sym=value: " << I;
+static int fillCommandLineSymbols(MCAsmParser &Parser) {
+  for (auto &I: DefineSymbol)
+    if (Parser.getContext().setSymbolValue(Parser.getStreamer(), I))
       return 1;
-    }
-    int64_t Value;
-    if(Pair.second.getAsInteger(0, Value)){
-      errs() << "error: Value is not an integer: " << Pair.second;
-      return 1;
-    }
-    auto &Context = Parser.getContext();
-    auto Symbol = Context.getOrCreateSymbol(Pair.first);
-    Parser.getStreamer().EmitAssignment(Symbol,
-                                        MCConstantExpr::create(Value, Context));
-  }
   return 0;
 }
 
 static int AssembleInput(const char *ProgName, const Target *TheTarget,
                          SourceMgr &SrcMgr, MCContext &Ctx, MCStreamer &Str,
                          MCAsmInfo &MAI, MCSubtargetInfo &STI,
                          MCInstrInfo &MCII, MCTargetOptions &MCOptions) {
   std::unique_ptr<MCAsmParser> Parser(