[PATCH] D26214: [llvm] Implement support for -defsym assembler option

Jim Grosbach via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Dec 1 17:25:13 PST 2016


grosbach added inline comments.


================
Comment at: llvm/trunk/lib/MC/MCContext.cpp:263
 
+int MCContext::setSymbolValue(MCStreamer &Streamer, std::string &I) {
+    auto Pair = StringRef(I).split('=');
----------------
echristo wrote:
> mgrang wrote:
> > grosbach wrote:
> > > The error checking shouldn't be in the setter function, but rather in the caller.
> > @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.
> 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.
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.


================
Comment at: llvm/trunk/lib/MC/MCContext.cpp:266
+    if (Pair.second.empty()) {
+      errs() << "error: defsym must be of the form: sym=value: " << I << "\n";
+      return 1;
----------------
echristo wrote:
> mgrang wrote:
> > grosbach wrote:
> > > The MC layer should not be writing directly to errs(), but rather going through the diagnostics handlers.
> > @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.
> 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.
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. 


Repository:
  rL LLVM

https://reviews.llvm.org/D26214





More information about the llvm-commits mailing list