[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/lib/MC/MCDwarf.cpp

unsigned MCDwarfLineTableHeader::getFile(StringRef &Directory,
                                          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.
   if (!File.Name.empty())
     return 0;
 
   // If any files have an MD5 checksum, they all must.
-  if (FileNumber > 1)
-    assert(HasMD5 == (Checksum != nullptr));
+  if (HasMD5 != (Checksum != nullptr))
+    return UINT_MAX;
 
   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/lib/MC/MCParser/AsmParser.cpp

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 {
+      FileNumber = getStreamer().EmitDwarfFileDirective(FileNumber, Directory,
+                                                        Filename, CKMem);
+      if (check(FileNumber == 0, FileNumberLoc,
+                "file number already allocated") ||
+          check(FileNumber == UINT_MAX, DirectiveLoc,
+                "inconsistent use of MD5 checksums"))
+        return true;
+    }
   }
 
   return false;
 }
 
 /// parseDirectiveLine
 /// ::= .line [number]
 bool AsmParser::parseDirectiveLine() {

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

 
 # 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"