[ELF] Emit diagnostic for invalid size directive

[bcain]

Summary

If a .size directive which uses an expression to calculate the size has a typo
in a label reference or references a label from another section, it is invalid.
Emit an error instead of a fatal error, and if there's a way to hint at the
cause, emit relevant diagnostic notes.

[MaskRay]

Is it overkill to add a spell corrector? .size is usually generated by the compiler, so this can only be useful for hand-written assembly. Though I wonder how many times this can be useful.

[bcain]

In D101068#2714208, @MaskRay wrote:

Is it overkill to add a spell corrector? .size is usually generated by the compiler, so this can only be useful for hand-written assembly. Though I wonder how many times this can be useful.

For whatever reason, many hexagon developers frequently use the .size directive to set the function size correctly when hand-writing assembly. There may be / may have been some developer scripts that expected/relied on this being set.

This particular patch is inspired by developer bug reports - including one that had a .size referencing a label incorrectly - with a single-character error: differing only in case.

If you think the "did you mean..." is overkill, I am happy to remove it.

[MaskRay]

Yeah, I kinda feel like we are not making good use of the 50 lines of code. .size is too minor a feature that I'd waste 50 lines, adding MCAssembler &Asm, in various places to make this work.
To make good use of the 50 lines, this should be something generic (think as a utility similar to evaluateAsRelocatable), so that every other directive which has similar usage to .size can benefit from this.

[bcain]

In D101068#2718612, @MaskRay wrote:

Yeah, I kinda feel like we are not making good use of the 50 lines of code. .size is too minor a feature that I'd waste 50 lines, adding MCAssembler &Asm, in various places to make this work.
To make good use of the 50 lines, this should be something generic (think as a utility similar to evaluateAsRelocatable), so that every other directive which has similar usage to .size can benefit from this.

Hmm. Removing the spell check portion of NoteUndef only saves around ~9 lines. So it sounds like that's not sufficient? You prefer either not accepting any of this patch or expanding its scope to include other directives? I will explore the latter.

[MaskRay]

In D101068#2719743, @bcain wrote:

In D101068#2718612, @MaskRay wrote:

Yeah, I kinda feel like we are not making good use of the 50 lines of code. .size is too minor a feature that I'd waste 50 lines, adding MCAssembler &Asm, in various places to make this work.
To make good use of the 50 lines, this should be something generic (think as a utility similar to evaluateAsRelocatable), so that every other directive which has similar usage to .size can benefit from this.

Hmm. Removing the spell check portion of NoteUndef only saves around ~9 lines. So it sounds like that's not sufficient? You prefer either not accepting any of this patch or expanding its scope to include other directives? I will explore the latter.

Yes, reusing this in more places can improve its value... I am also a bit unsure about did you mean to refer suggesting more than one symbol.

llvm/lib/MC/ELFObjectWriter.cpp

 #include "llvm/Support/Compression.h"
 #include "llvm/Support/EndianStream.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/Host.h"
 #include "llvm/Support/LEB128.h"
 #include "llvm/Support/MathExtras.h"
 #include "llvm/Support/SMLoc.h"
+#include "llvm/Support/SourceMgr.h"
 #include "llvm/Support/StringSaver.h"
 #include "llvm/Support/SwapByteOrder.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 #include <cassert>
 #include <cstddef>
 #include <cstdint>
 #include <map>
public:
   }
 
   template <typename T> void write(T Val) {
     W.write(Val);
   }
 
   void writeHeader(const MCAssembler &Asm);
 
-  void writeSymbol(SymbolTableWriter &Writer, uint32_t StringIndex,
-                   ELFSymbolData &MSD, const MCAsmLayout &Layout);
+
+  void writeSymbol(MCAssembler &Asm, SymbolTableWriter &Writer,
+                   uint32_t StringIndex, ELFSymbolData &MSD,
+                   const MCAsmLayout &Layout);
+
+  void reportInvalidSize(MCAssembler &Asm, const MCAsmLayout &Layout,
+                         const MCExpr *SizeExpr);
 
   // Start and end offset of each section
   using SectionOffsetsTy =
       std::map<const MCSectionELF *, std::pair<uint64_t, uint64_t>>;
 
   // Map from a signature symbol to the group section index
   using RevGroupMapTy = DenseMap<const MCSymbol *, unsigned>;
 
if (!Symbol->isVariable() ||
         Value->getKind() != MCSymbolRefExpr::VK_None ||
         mergeTypeForSet(Symbol->getType(), ELF::STT_GNU_IFUNC) != ELF::STT_GNU_IFUNC)
       return false;
     Symbol = &cast<MCSymbolELF>(Value->getSymbol());
   }
   return true;
 }
 
-void ELFWriter::writeSymbol(SymbolTableWriter &Writer, uint32_t StringIndex,
-                            ELFSymbolData &MSD, const MCAsmLayout &Layout) {
+void ELFWriter::writeSymbol(MCAssembler &Asm, SymbolTableWriter &Writer,
+                            uint32_t StringIndex, ELFSymbolData &MSD,
+                            const MCAsmLayout &Layout) {
   const auto &Symbol = cast<MCSymbolELF>(*MSD.Symbol);
   const MCSymbolELF *Base =
       cast_or_null<MCSymbolELF>(Layout.getBaseSymbol(Symbol));
 
   // This has to be in sync with when computeSymbolTable uses SHN_ABS or
   // SHN_COMMON.
   bool IsReserved = !Base || Symbol.isCommon();
 
void ELFWriter::writeSymbol(MCAssembler &Asm, SymbolTableWriter &Writer,
   uint64_t Size = 0;
 
   const MCExpr *ESize = MSD.Symbol->getSize();
   if (!ESize && Base)
     ESize = Base->getSize();
 
   if (ESize) {
     int64_t Res;
-    if (!ESize->evaluateKnownAbsolute(Res, Layout))
-      report_fatal_error("Size expression must be absolute.");
+    if (!ESize->evaluateKnownAbsolute(Res, Layout)) {
+      reportInvalidSize(Asm, Layout, ESize);
+      return;
+    }
     Size = Res;
   }
 
   // Write out the symbol table entry
   Writer.writeSymbol(StringIndex, Info, Value, Size, Other, MSD.SectionIndex,
                      IsReserved);
 }
 
+void ELFWriter::reportInvalidSize(MCAssembler &Asm, const MCAsmLayout &Layout,
+                                  const MCExpr *SizeExpr) {
+  MCContext &Ctx = Asm.getContext();
+  SMLoc SizeLoc = SizeExpr->getLoc();
+  Ctx.reportError(SizeLoc, "size expression is not absolute");
+  const SourceMgr *SM = Ctx.getSourceManager();
+
+  if (!SM)
+    return;
+
+  auto NoteNotInSect = [&SM, &SizeLoc](MCSymbol const &SymRef) {
+    SM->PrintMessage(SizeLoc, SourceMgr::DK_Note,
+                     Twine("symbol '") + SymRef.getName() +
+                         "' is not defined in this section");
+  };
+  auto NoteUndef = [&SM, &Asm](MCSymbol const &SymRef, SMLoc Loc) {
+    SM->PrintMessage(Loc, SourceMgr::DK_Note, "symbol not defined");
+
+    const unsigned MaxEditDist = 2;
+    for (const MCSymbol &Sym : Asm.symbols()) {
+      StringRef Name = Sym.getName();
+      if (Name.edit_distance(SymRef.getName()) < MaxEditDist)
+        SM->PrintMessage(Loc, SourceMgr::DK_Note,
+                         Twine("did you mean to refer to '") + Name +
+                             Twine("'?"));
+    }
+  };
+  auto NoteSym = [=](MCSymbol const &Sym, SMLoc Loc) {
+    if (Sym.isUndefined())
+      NoteUndef(Sym, Loc);
+    else if (Sym.isInSection() && !Sym.getName().empty())
+      NoteNotInSect(Sym);
+  };
+
+  if (const MCSymbolRefExpr *Ref = dyn_cast<MCSymbolRefExpr>(SizeExpr)) {
+    NoteSym(Ref->getSymbol(), Ref->getLoc());
+  } else if (SizeExpr->getKind() == MCExpr::Binary) {
+    const MCBinaryExpr *BE = static_cast<const MCBinaryExpr *>(SizeExpr);
+    MCValue V;
+
+    if (BE->evaluateAsRelocatable(V, &Layout, nullptr)) {
+      if (V.getSymA())
+        NoteSym(V.getSymA()->getSymbol(), V.getSymA()->getLoc());
+      if (V.getSymB())
+        NoteSym(V.getSymB()->getSymbol(), V.getSymB()->getLoc());
+    }
+  }
+}
+
 bool ELFWriter::isInSymtab(const MCAsmLayout &Layout, const MCSymbolELF &Symbol,
                            bool Used, bool Renamed) {
   if (Symbol.isVariable()) {
     const MCExpr *Expr = Symbol.getVariableValue();
     // Target Expressions that are always inlined do not appear in the symtab
     if (const auto *T = dyn_cast<MCTargetExpr>(Expr))
       if (T->inlineAssignedExpr())
         return false;
for (; FileNameIt != FileNames.end() && FileNameIt->second <= MSD.Order;
                          ELF::SHN_ABS, true);
       ++Index;
     }
 
     unsigned StringIndex = MSD.Symbol->getType() == ELF::STT_SECTION
                                ? 0
                                : StrTabBuilder.getOffset(MSD.Name);
     MSD.Symbol->setIndex(Index++);
-    writeSymbol(Writer, StringIndex, MSD, Layout);
+    writeSymbol(Asm, Writer, StringIndex, MSD, Layout);
   }
   for (; FileNameIt != FileNames.end(); ++FileNameIt) {
     Writer.writeSymbol(StrTabBuilder.getOffset(FileNameIt->first),
                        ELF::STT_FILE | ELF::STB_LOCAL, 0, 0, ELF::STV_DEFAULT,
                        ELF::SHN_ABS, true);
     ++Index;
   }
 
   // Write the symbol table entries.
   LastLocalSymbolIndex = Index;
 
   for (ELFSymbolData &MSD : ExternalSymbolData) {
     unsigned StringIndex = StrTabBuilder.getOffset(MSD.Name);
     MSD.Symbol->setIndex(Index++);
-    writeSymbol(Writer, StringIndex, MSD, Layout);
+    writeSymbol(Asm, Writer, StringIndex, MSD, Layout);
     assert(MSD.Symbol->getBinding() != ELF::STB_LOCAL);
   }
 
   uint64_t SecEnd = W.OS.tell();
   SectionOffsets[SymtabSection] = std::make_pair(SecStart, SecEnd);
 
   ArrayRef<uint32_t> ShndxIndexes = Writer.getShndxIndexes();
   if (ShndxIndexes.empty()) {

llvm/test/MC/ELF/size-error.s

+// RUN: not llvm-mc -filetype=obj -triple x86_64-pc-linux < %s 2>&1 \
+// RUN: | FileCheck --implicit-check-not=error %s
+
+.section some
+
+Bar:
+nop
+
+.section other
+
+foo:
+  .size foo, fo
+// CHECK: error: size expression is not absolute
+// CHECK: note: symbol not defined
+// CHECK: note: did you mean to refer to 'foo'?
+
+ethers:
+    nop
+  .size ethers, Bar + 42
+// CHECK: error: size expression is not absolute
+// CHECK: note: symbol 'Bar' is not defined in this section
+
+a:
+  .size a, . - others
+// CHECK: error: size expression is not absolute
+// CHECK: note: symbol not defined
+// CHECK: note: did you mean to refer to 'other'?
+// CHECK: note: did you mean to refer to 'ethers'?
+
+bar:
+  .size bar, 31 - baz
+
+// CHECK: error: size expression is not absolute
+// CHECK: note: symbol not defined
+// CHECK: note: did you mean to refer to 'bar'?