[DWARFv5] Turn an assert into a diagnostic.

[probinson]

Summary

Hand-coded assembler files should not trigger assertions.

I'm not really keen on my choice of error indicators, which is why I'm
posting this for review; suggestions welcome. The thing is, there's a
moderate amount of plumbing in between the AsmParser and the DWARF
file table management that would be affected, that is also used by
DwarfDebug, and changing it would have largely no benefit.
I contemplated ErrorOr<> for a while, but it's not that easy to use
for a non-syscall diagnostic.

[dblaikie]

Not sure what you mean by "a non-syscall diagnostic"? (there's StringError if you want to just put a string rather than an error code)

What is it that's undesirable about the error handling you've implemented here? Given that there's a similar error path nearby, that seems like a reasonable (or at least consistent) approach... but I've not looked at this area of the code in detail to see if there's something particularly undesirable about it.

[probinson]

In D43152#1004647, @dblaikie wrote:

Not sure what you mean by "a non-syscall diagnostic"? (there's StringError if you want to just put a string rather than an error code)

I mean, a case where a std::error_code is not what we want.

What is it that's undesirable about the error handling you've implemented here? Given that there's a similar error path nearby, that seems like a reasonable (or at least consistent) approach... but I've not looked at this area of the code in detail to see if there's something particularly undesirable about it.

Overloading two return values to indicate an error, instead of a more explicit indication. And then relying on callers to remember to check for them, instead of forcing callers to check for them.

[probinson]

Use Expected<unsigned> instead of just unsigned to return errors.

[dblaikie]

Given the abundance of callers that know the call won't fail, perhaps it'd be appropriate to keep the original function name without Expected, then have a version of the function ('tryBlah') that returns Expected (& implement blah in terms of cantFail(tryBlah))

[probinson]

In D43152#1013279, @dblaikie wrote:

Given the abundance of callers that know the call won't fail, perhaps it'd be appropriate to keep the original function name without Expected, then have a version of the function ('tryBlah') that returns Expected (& implement blah in terms of cantFail(tryBlah))

That would certainly be less churn, and I'm happy to go that way.

llvm/trunk/include/llvm/MC/MCContext.h

 #include "llvm/ADT/Twine.h"
 #include "llvm/BinaryFormat/Dwarf.h"
 #include "llvm/MC/MCAsmMacro.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/Error.h"
 #include "llvm/Support/MD5.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 #include <cassert>
 #include <cstddef>
 #include <cstdint>
 #include <map>
 #include <memory>
public:
     /// info. This can be set to ensure we've got the correct file name
     /// after preprocessing or for -save-temps.
     const std::string &getMainFileName() const { return MainFileName; }
 
     /// \brief Set the main file name and override the default.
     void setMainFileName(StringRef S) { MainFileName = S; }
 
     /// Creates an entry in the dwarf file and directory tables.
-    unsigned getDwarfFile(StringRef Directory, StringRef FileName,
-                          unsigned FileNumber, MD5::MD5Result *Checksum,
-                          unsigned CUID);
+    Expected<unsigned> getDwarfFile(StringRef Directory, StringRef FileName,
+                                    unsigned FileNumber,
+                                    MD5::MD5Result *Checksum, unsigned CUID);
 
     bool isValidDwarfFileNumber(unsigned FileNumber, unsigned CUID = 0);
 
     const std::map<unsigned, MCDwarfLineTable> &getMCDwarfLineTables() const {
       return MCDwarfLineTablesCUMap;
     }
 
     MCDwarfLineTable &getMCDwarfLineTable(unsigned CUID) {

llvm/trunk/include/llvm/MC/MCDwarf.h

 #ifndef LLVM_MC_MCDWARF_H
 #define LLVM_MC_MCDWARF_H
 
 #include "llvm/ADT/MapVector.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/MC/MCSection.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/MD5.h"
 #include <cassert>
 #include <cstdint>
 #include <string>
 #include <utility>
 #include <vector>
 
 namespace llvm {
struct MCDwarfLineTableHeader {
   SmallVector<std::string, 3> MCDwarfDirs;
   SmallVector<MCDwarfFile, 3> MCDwarfFiles;
   StringMap<unsigned> SourceIdMap;
   StringRef CompilationDir;
   bool HasMD5 = false;
 
   MCDwarfLineTableHeader() = default;
 
-  unsigned getFile(StringRef &Directory, StringRef &FileName,
-                   MD5::MD5Result *Checksum, unsigned FileNumber = 0);
+  Expected<unsigned> tryGetFile(StringRef &Directory, StringRef &FileName,
+                                MD5::MD5Result *Checksum,
+                                unsigned FileNumber = 0);
   std::pair<MCSymbol *, MCSymbol *>
   Emit(MCStreamer *MCOS, MCDwarfLineTableParams Params,
        Optional<MCDwarfLineStr> &LineStr) const;
   std::pair<MCSymbol *, MCSymbol *>
   Emit(MCStreamer *MCOS, MCDwarfLineTableParams Params,
        ArrayRef<char> SpecialOpcodeLengths,
        Optional<MCDwarfLineStr> &LineStr) const;
 
 private:
   void emitV2FileDirTables(MCStreamer *MCOS) const;
   void emitV5FileDirTables(MCStreamer *MCOS,
                            Optional<MCDwarfLineStr> &LineStr) const;
 };
 
 class MCDwarfDwoLineTable {
   MCDwarfLineTableHeader Header;
 
 public:
   void setCompilationDir(StringRef CompilationDir) {
     Header.CompilationDir = CompilationDir;
   }
 
   unsigned getFile(StringRef Directory, StringRef FileName,
                    MD5::MD5Result *Checksum) {
-    return Header.getFile(Directory, FileName, Checksum);
+    return cantFail(Header.tryGetFile(Directory, FileName, Checksum));
   }
 
   void Emit(MCStreamer &MCOS, MCDwarfLineTableParams Params) const;
 };
 
 class MCDwarfLineTable {
   MCDwarfLineTableHeader Header;
   MCLineSection MCLineSections;
 
 public:
   // This emits the Dwarf file and the line tables for all Compile Units.
   static void Emit(MCObjectStreamer *MCOS, MCDwarfLineTableParams Params);
 
   // This emits the Dwarf file and the line tables for a given Compile Unit.
   void EmitCU(MCObjectStreamer *MCOS, MCDwarfLineTableParams Params,
               Optional<MCDwarfLineStr> &LineStr) const;
 
+  Expected<unsigned> tryGetFile(StringRef &Directory, StringRef &FileName,
+                                MD5::MD5Result *Checksum,
+                                unsigned FileNumber = 0);
   unsigned getFile(StringRef &Directory, StringRef &FileName,
-                   MD5::MD5Result *Checksum, unsigned FileNumber = 0);
+                   MD5::MD5Result *Checksum, unsigned FileNumber = 0) {
+    return cantFail(tryGetFile(Directory, FileName, Checksum, FileNumber));
+  }
 
   MCSymbol *getLabel() const {
     return Header.Label;
   }
 
   void setLabel(MCSymbol *Label) {
     Header.Label = Label;
   }

llvm/trunk/include/llvm/MC/MCStreamer.h

 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/MC/MCDirectives.h"
 #include "llvm/MC/MCDwarf.h"
 #include "llvm/MC/MCLinkerOptimizationHint.h"
 #include "llvm/MC/MCSymbol.h"
 #include "llvm/MC/MCWinEH.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/MD5.h"
 #include "llvm/Support/SMLoc.h"
 #include "llvm/Support/TargetParser.h"
 #include <cassert>
 #include <cstdint>
 #include <memory>
 #include <string>
 #include <utility>
public:
   virtual void EmitFileDirective(StringRef Filename);
 
   /// \brief Emit the "identifiers" directive.  This implements the
   /// '.ident "version foo"' assembler directive.
   virtual void EmitIdent(StringRef IdentString) {}
 
   /// \brief Associate a filename with a specified logical file number.  This
   /// implements the DWARF2 '.file 4 "foo.c"' assembler directive.
-  virtual unsigned EmitDwarfFileDirective(unsigned FileNo, StringRef Directory,
-                                          StringRef Filename,
-                                          MD5::MD5Result *Checksum = nullptr,
-                                          unsigned CUID = 0);
+  unsigned EmitDwarfFileDirective(unsigned FileNo, StringRef Directory,
+                                  unsigned CUID = 0) {
+    return cantFail(
+        tryEmitDwarfFileDirective(FileNo, Directory, Filename, Checksum, CUID));
+  }
+
+  /// Associate a filename with a specified logical file number.  Returns a
+  /// StringError for any problem, or the file number.  This implements the
+  /// DWARF2 '.file 4 "foo.c"' assembler directive.
+  virtual Expected<unsigned> tryEmitDwarfFileDirective(
+      unsigned FileNo, StringRef Directory, StringRef Filename,
+      MD5::MD5Result *Checksum = nullptr, unsigned CUID = 0);
 
   /// \brief This implements the DWARF2 '.loc fileno lineno ...' assembler
   /// directive.
   virtual void EmitDwarfLocDirective(unsigned FileNo, unsigned Line,
                                      unsigned Column, unsigned Flags,
                                      unsigned Isa, unsigned Discriminator,
                                      StringRef FileName);
 

llvm/trunk/lib/MC/MCAsmStreamer.cpp

public:
   void EmitCodeAlignment(unsigned ByteAlignment,
                          unsigned MaxBytesToEmit = 0) override;
 
   void emitValueToOffset(const MCExpr *Offset,
                          unsigned char Value,
                          SMLoc Loc) override;
 
   void EmitFileDirective(StringRef Filename) override;
-  unsigned EmitDwarfFileDirective(unsigned FileNo, StringRef Directory,
+  Expected<unsigned> tryEmitDwarfFileDirective(unsigned FileNo,
+                                               StringRef Directory,
                                                StringRef Filename,
                                                MD5::MD5Result *Checksum = 0,
                                                unsigned CUID = 0) override;
   void EmitDwarfLocDirective(unsigned FileNo, unsigned Line,
                              unsigned Column, unsigned Flags,
                              unsigned Isa, unsigned Discriminator,
                              StringRef FileName) override;
   MCSymbol *getDwarfLineTableSymbol(unsigned CUID) override;
 
   bool EmitCVFileDirective(unsigned FileNo, StringRef Filename,
                            ArrayRef<uint8_t> Checksum,

 
 void MCAsmStreamer::EmitFileDirective(StringRef Filename) {
   assert(MAI->hasSingleParameterDotFile());
   OS << "\t.file\t";
   PrintQuotedString(Filename, OS);
   EmitEOL();
 }
 
-unsigned MCAsmStreamer::EmitDwarfFileDirective(unsigned FileNo,
-                                               StringRef Directory,
-                                               StringRef Filename,
-                                               MD5::MD5Result *Checksum,
-                                               unsigned CUID) {
+Expected<unsigned> MCAsmStreamer::tryEmitDwarfFileDirective(
+    unsigned FileNo, StringRef Directory, StringRef Filename,
+    MD5::MD5Result *Checksum, unsigned CUID) {
   assert(CUID == 0);
 
   MCDwarfLineTable &Table = getContext().getMCDwarfLineTable(CUID);
   unsigned NumFiles = Table.getMCDwarfFiles().size();
-  FileNo = Table.getFile(Directory, Filename, Checksum, FileNo);
-  if (FileNo == 0)
-    return 0;
+  Expected<unsigned> FileNoOrErr =
+      Table.tryGetFile(Directory, Filename, Checksum, FileNo);
+  if (!FileNoOrErr)
+    return FileNoOrErr.takeError();
+  FileNo = FileNoOrErr.get();
   if (NumFiles == Table.getMCDwarfFiles().size())
     return FileNo;
 
   SmallString<128> FullPathName;
 
   if (!UseDwarfDirectory && !Directory.empty()) {
     if (sys::path::is_absolute(Filename))
       Directory = "";

llvm/trunk/lib/MC/MCContext.cpp

 //===----------------------------------------------------------------------===//
 // Dwarf Management
 //===----------------------------------------------------------------------===//
 
 /// getDwarfFile - takes a file name an number to place in the dwarf file and
 /// directory tables.  If the file number has already been allocated it is an
 /// error and zero is returned and the client reports the error, else the
 /// allocated file number is returned.  The file numbers may be in any order.
-unsigned MCContext::getDwarfFile(StringRef Directory, StringRef FileName,
-                                 unsigned FileNumber, MD5::MD5Result *Checksum,
+Expected<unsigned> MCContext::getDwarfFile(StringRef Directory,
+                                           StringRef FileName,
+                                           unsigned FileNumber,
+                                           MD5::MD5Result *Checksum,
                                            unsigned CUID) {
   MCDwarfLineTable &Table = MCDwarfLineTablesCUMap[CUID];
-  return Table.getFile(Directory, FileName, Checksum, FileNumber);
+  return Table.tryGetFile(Directory, FileName, Checksum, FileNumber);
 }
 
 /// isValidDwarfFileNumber - takes a dwarf file number and returns true if it
 /// currently is assigned and false otherwise.
 bool MCContext::isValidDwarfFileNumber(unsigned FileNumber, unsigned CUID) {
   const SmallVectorImpl<MCDwarfFile> &MCDwarfFiles = getMCDwarfFiles(CUID);
   if (FileNumber == 0 || FileNumber >= MCDwarfFiles.size())
     return false;

llvm/trunk/lib/MC/MCDwarf.cpp

void MCDwarfLineTable::EmitCU(MCObjectStreamer *MCOS,
   for (const auto &LineSec : MCLineSections.getMCLineEntries())
     EmitDwarfLineTable(MCOS, LineSec.first, LineSec.second);
 
   // This is the end of the section, so set the value of the symbol at the end
   // of this section (that was used in a previous expression).
   MCOS->EmitLabel(LineEndSym);
 }
 
-unsigned MCDwarfLineTable::getFile(StringRef &Directory, StringRef &FileName,
+Expected<unsigned> MCDwarfLineTable::tryGetFile(StringRef &Directory,
+                                                StringRef &FileName,
                                                 MD5::MD5Result *Checksum,
                                                 unsigned FileNumber) {
-  return Header.getFile(Directory, FileName, Checksum, FileNumber);
+  return Header.tryGetFile(Directory, FileName, Checksum, FileNumber);
 }
 
-unsigned MCDwarfLineTableHeader::getFile(StringRef &Directory,
+Expected<unsigned> MCDwarfLineTableHeader::tryGetFile(StringRef &Directory,
                                                       StringRef &FileName,
                                                       MD5::MD5Result *Checksum,
                                                       unsigned FileNumber) {
   if (Directory == CompilationDir)
     Directory = "";
   if (FileName.empty()) {
     FileName = "<stdin>";
     Directory = "";
   }
   assert(!FileName.empty());
+  // If any files have an MD5 checksum, they all must.
+  if (MCDwarfFiles.empty())
+    HasMD5 = (Checksum != nullptr);
   if (FileNumber == 0) {
     // File numbers start with 1 and/or after any file numbers
     // allocated by inline-assembler .file directives.
     FileNumber = MCDwarfFiles.empty() ? 1 : MCDwarfFiles.size();
     SmallString<256> Buffer;
     auto IterBool = SourceIdMap.insert(
         std::make_pair((Directory + Twine('\0') + FileName).toStringRef(Buffer),
                        FileNumber));
     if (!IterBool.second)
       return IterBool.first->second;
   }
   // Make space for this FileNumber in the MCDwarfFiles vector if needed.
   if (FileNumber >= MCDwarfFiles.size())
     MCDwarfFiles.resize(FileNumber + 1);
 
   // Get the new MCDwarfFile slot for this FileNumber.
   MCDwarfFile &File = MCDwarfFiles[FileNumber];
 
-  // It is an error to use see the same number more than once.
+  // It is an error to see the same number more than once.
   if (!File.Name.empty())
-    return 0;
+    return make_error<StringError>("file number already allocated",
+                                   inconvertibleErrorCode());
 
   // If any files have an MD5 checksum, they all must.
-  if (FileNumber > 1)
-    assert(HasMD5 == (Checksum != nullptr));
+  if (HasMD5 != (Checksum != nullptr))
+    return make_error<StringError>("inconsistent use of MD5 checksums",
+                                   inconvertibleErrorCode());
 
   if (Directory.empty()) {
     // Separate the directory part from the basename of the FileName.
     StringRef tFileName = sys::path::filename(FileName);
     if (!tFileName.empty()) {
       Directory = sys::path::parent_path(FileName);
       if (!Directory.empty())
         FileName = tFileName;

llvm/trunk/lib/MC/MCParser/AsmParser.cpp

 }
 
 /// parseDirectiveFile
 /// ::= .file filename
 /// ::= .file number [directory] filename [md5 checksum]
 bool AsmParser::parseDirectiveFile(SMLoc DirectiveLoc) {
   // FIXME: I'm not sure what this is.
   int64_t FileNumber = -1;
-  SMLoc FileNumberLoc = getLexer().getLoc();
   if (getLexer().is(AsmToken::Integer)) {
     FileNumber = getTok().getIntVal();
     Lex();
 
     if (FileNumber < 1)
       return TokError("file number less than one");
   }
 
if (!Checksum.empty()) {
         return true;
       CKMem = (MD5::MD5Result *)Ctx.allocate(sizeof(MD5::MD5Result), 1);
       memcpy(&CKMem->Bytes, Checksum.data(), 16);
     }
     // If there is -g option as well as debug info from directive file,
     // we turn off -g option, directly use the existing debug info instead.
     if (getContext().getGenDwarfForAssembly())
       getContext().setGenDwarfForAssembly(false);
-    else if (getStreamer().EmitDwarfFileDirective(FileNumber, Directory,
-                                                  Filename, CKMem) == 0)
-      return Error(FileNumberLoc, "file number already allocated");
+    else {
+      Expected<unsigned> FileNumOrErr = getStreamer().tryEmitDwarfFileDirective(
+          FileNumber, Directory, Filename, CKMem);
+      if (!FileNumOrErr)
+        return Error(DirectiveLoc, toString(FileNumOrErr.takeError()));
+      FileNumber = FileNumOrErr.get();
+    }
   }
 
   return false;
 }
 
 /// parseDirectiveLine
 /// ::= .line [number]
 bool AsmParser::parseDirectiveLine() {

llvm/trunk/lib/MC/MCStreamer.cpp

void MCStreamer::emitFill(uint64_t NumBytes, uint8_t FillValue) {
   emitFill(*MCConstantExpr::create(NumBytes, getContext()), FillValue);
 }
 
 /// The implementation in this class just redirects to emitFill.
 void MCStreamer::EmitZeros(uint64_t NumBytes) {
   emitFill(NumBytes, 0);
 }
 
-unsigned MCStreamer::EmitDwarfFileDirective(unsigned FileNo,
-                                            StringRef Directory,
-                                            StringRef Filename,
-                                            MD5::MD5Result *Checksum,
-                                            unsigned CUID) {
+Expected<unsigned>
+MCStreamer::tryEmitDwarfFileDirective(unsigned FileNo, StringRef Directory,
+                                      MD5::MD5Result *Checksum, unsigned CUID) {
   return getContext().getDwarfFile(Directory, Filename, FileNo, Checksum, CUID);
 }
 
 void MCStreamer::EmitDwarfLocDirective(unsigned FileNo, unsigned Line,
                                        unsigned Column, unsigned Flags,
                                        unsigned Isa,
                                        unsigned Discriminator,
                                        StringRef FileName) {

llvm/trunk/test/MC/ELF/debug-md5-err.s

 # RUN: not llvm-mc -triple x86_64-unknown-unknown -dwarf-version 5 -filetype=asm %s -o /dev/null 2>&1 | FileCheck %s
+# RUN: not llvm-mc -triple x86_64-unknown-unknown -dwarf-version 5 -filetype=obj %s -o /dev/null 2>&1 | FileCheck %s
 
 # This is syntactically legal, looks like no checksum provided.
 # CHECK-NOT: [[@LINE+1]]:{{[0-9]+}}: error:
         .file 1 "dir1/foo" "00112233445566778899aabbccddeeff"
 
 # Missing md5 keyword.
 # CHECK: [[@LINE+1]]:{{[0-9]+}}: error: unexpected token in '.file' directive
         .file 2 "dir1" "foo" "00112233445566778899aabbccddeeff"
 
 # Bad length.
 # CHECK: [[@LINE+1]]:{{[0-9]+}}: error: invalid MD5 checksum specified
         .file 3 "dir2" "bar" md5 "ff"
 
 # Not a string.
 # CHECK: [[@LINE+1]]:{{[0-9]+}}: error: unexpected token in '.file' directive
         .file 4 "dir3" "foo" md5 ffeeddccbbaa99887766554433221100
 
 # Non-DWARF .file syntax with checksum.
 # CHECK: [[@LINE+1]]:{{[0-9]+}}: error: MD5 checksum specified, but no file number
         .file "baz" md5 "ffeeddccbbaa998877665544332211gg"
+
+# Inconsistent use of MD5 option. Note: .file 1 did not supply one.
+# CHECK: [[@LINE+1]]:{{[0-9]+}}: error: inconsistent use of MD5 checksums
+        .file 5 "bax" md5 "ffeeddccbbaa99887766554433221100"