PECOFF: consume errors properly

[compnerd]

Summary

We would not ensure that the error is consumed in the case that logging
is disabled. Ensure that we properly drop the error on the floor or we
would re-trigger the checked failure.

[bulbazord]

lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp

865–874

If logging is enabled, we are moving from the same object twice, no?

I think we should rethink the LLDB_LOG macro when it comes to errors.... :/

[compnerd]

lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp

865–874

Yeah ... I was worried about that.

[compnerd]

lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp

865–874

I should mention that at least on MSVC I _can_ get away with it:

ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry name: RVA 0x0 for export ordinal table not found
ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry name: RVA 0x0 for export ordinal table not found
ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry name: RVA 0x0 for export ordinal table not found
ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry name: RVA 0x0 for export ordinal table not found
ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry name: RVA 0x0 for export ordinal table not found
ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry name: RVA 0x0 for export ordinal table not found 
ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry name: RVA 0x0 for export ordinal table not found
ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry name: RVA 0x0 for export ordinal table not found
ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry name: RVA 0x0 for export ordinal table not found
ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry name: RVA 0x0 for export ordinal table not found
ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry name: RVA 0x0 for export ordinal table not found
ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry name: RVA 0x0 for export ordinal table not found
ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry name: RVA 0x0 for export ordinal table not found
ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry name: RVA 0x0 for export ordinal table not found
ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry name: RVA 0x0 for export ordinal table not found
ObjectFilePECOFF::AppendFromExportTable - failed to get export table entry name: RVA 0x0 for export ordinal table not found

[compnerd]

lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp

865–874

https://godbolt.org/z/nj4r7K8hb

UBSAN also seems to indicate it is permissible.

[bulbazord]

lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp

865–874

Moving from a standard library type leaves the original object in a "valid but unspecified" state. Do we make such guarantees about llvm::Errors? This is probably going to work "correctly" but I'm not sure what might go wrong.

[sgraenitz]

First of all, yes the existing code is incorrect. Thanks for looking into this. However, I am afraid this isn't the right solution.

In the if body you can not std::move() from err twice. I guess we have to do something like:

if (log)
  LLDB_LOG(...)
else
  consumeError(...)

which resulted in a check failure under LLVM_ENABLE_ABI_BREAKING_CHANGES

You probably mean LLVM_ENABLE_ABI_BREAKING_CHECKS? For Error this enables logic to make sure the error was checked: https://github.com/llvm/llvm-project/blob/release/16.x/llvm/include/llvm/Support/Error.h#L724 I think your observation is a side effect of the issue you look at and has nothing to do with compiler optimizations like NRVO. If logging is off, the error isn't consumed.

[alvinhochun]

Thanks for adding me in the loop. And thanks for looking into the fix, though this bit is a bit confusing to me:

Ensure that we explicitly indicate that we would like the move semantics
in the assignment. With MSVC 14.35.32215, the assignment would not
trigger a NVRO and copy constructor would be invoked

From https://llvm.org/doxygen/classllvm_1_1Error.html, llvm::Error has both copy constructor and copy assignment deleted, so I don't understand how a copy constructor could be invoked? I believe I followed https://llvm.org/docs/ProgrammersManual.html#recoverable-errors in handling the error, so if not having std::move there is really an issue, that page should also be updated.

But it is probably like @sgraenitz said, it is just an effect of the error not being consumed when logging is disabled.

If logging is enabled, we are moving from the same object twice, no?

In the if body you can not std::move() from err twice.

The doc comment of the move constructor/assignment does say "original error becomes a checked Success value, regardless of its original state" and it is exactly what happens in the code, so moving from the same error twice should be fine.

[compnerd]

In D147669#4249968, @sgraenitz wrote:

First of all, yes the existing code is incorrect. Thanks for looking into this. However, I am afraid this isn't the right solution.

You probably mean LLVM_ENABLE_ABI_BREAKING_CHECKS? For Error this enables logic to make sure the error was checked: https://github.com/llvm/llvm-project/blob/release/16.x/llvm/include/llvm/Support/Error.h#L724 I think your observation is a side effect of the issue you look at and has nothing to do with compiler optimizations like NRVO. If logging is off, the error isn't consumed.

Yes, I did indeed mean checks and not changes. I had originally tried with only the consumption in the non-logging case, but that still seemed to abort similarly. Adding the std::move on the assignment for the conditional was also required for some reason.

I do wonder if we should keep the consume to be unconditional or not as @alvinhochun points out that llvm::Error does guarantee this behaviour.

[compnerd]

In D147669#4251441, @compnerd wrote:

In D147669#4249968, @sgraenitz wrote:

First of all, yes the existing code is incorrect. Thanks for looking into this. However, I am afraid this isn't the right solution.

You probably mean LLVM_ENABLE_ABI_BREAKING_CHECKS? For Error this enables logic to make sure the error was checked: https://github.com/llvm/llvm-project/blob/release/16.x/llvm/include/llvm/Support/Error.h#L724 I think your observation is a side effect of the issue you look at and has nothing to do with compiler optimizations like NRVO. If logging is off, the error isn't consumed.

Yes, I did indeed mean checks and not changes. I had originally tried with only the consumption in the non-logging case, but that still seemed to abort similarly. Adding the std::move on the assignment for the conditional was also required for some reason.

Hmm, now I'm wondering if I had run the wrong binary, because I do remember that I somehow ended up running the wrong binary once or twice. Let's go with removing the confusing std::move in the if because that _shouldn't_ be needed.

[sgraenitz]

I think it's a very good practice that objects in moved-from state will only ever get destroyed. While the move ctor itself has no preconditions and thus might be fine to call again, the implementation of consumeError() does inspect the object. This adds an unnecessary risk for UB. Please simply check the log state and call either fmt_consume() or consumeError(). Thanks.

[compnerd]

Address feedback

[compnerd]

@sgraenitz done :)

[sgraenitz]

Great. Thanks!

lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp

if (num_syms == 0)
     return {};
 
   Log *log = GetLog(LLDBLog::Object);
   rva_symbol_list_t export_list;
   symtab.Reserve(symtab.GetNumSymbols() + num_syms);
   // Read each export table entry, ordered by ordinal instead of by name.
   for (const auto &entry : m_binary->export_directories()) {
     llvm::StringRef sym_name;
     if (auto err = entry.getSymbolName(sym_name)) {
-      LLDB_LOG(log,
-               "ObjectFilePECOFF::AppendFromExportTable - failed to get export "
-               "table entry name: {0}",
-               llvm::fmt_consume(std::move(err)));
+      if (log)
+        log->Format(
+            __FILE__, __func__,
+      else
+        llvm::consumeError(std::move(err));
       continue;
     }
     Symbol symbol;
     // Note: symbol name may be empty if it is only exported by ordinal.
     symbol.GetMangled().SetValue(ConstString(sym_name));
 
     uint32_t ordinal;
     llvm::cantFail(entry.getOrdinal(ordinal));
     symbol.SetID(ordinal);
 
     bool is_forwarder;
     llvm::cantFail(entry.isForwarder(is_forwarder));
     if (is_forwarder) {
       // Forwarder exports are redirected by the loader transparently, but keep
       // it in symtab and make a note using the symbol name.
       llvm::StringRef forwarder_name;
       if (auto err = entry.getForwardTo(forwarder_name)) {
-        LLDB_LOG(log,
+        if (log)
+          log->Format(__FILE__, __func__,
                       "ObjectFilePECOFF::AppendFromExportTable - failed to get "
                       "forwarder name of forwarder export '{0}': {1}",
                       sym_name, llvm::fmt_consume(std::move(err)));
+        else
+          llvm::consumeError(std::move(err));
         continue;
       }
       llvm::SmallString<256> new_name = {symbol.GetDisplayName().GetStringRef(),
                                          " (forwarded to ", forwarder_name,
                                          ")"};
       symbol.GetMangled().SetDemangledName(ConstString(new_name.str()));
       symbol.SetDemangledNameIsSynthesized(true);
     }
 
     uint32_t function_rva;
     if (auto err = entry.getExportRVA(function_rva)) {
-      LLDB_LOG(log,
+      if (log)
+        log->Format(__FILE__, __func__,
                     "ObjectFilePECOFF::AppendFromExportTable - failed to get "
                     "address of export entry '{0}': {1}",
                     sym_name, llvm::fmt_consume(std::move(err)));
+      else
+        llvm::consumeError(std::move(err));
       continue;
     }
     // Skip the symbol if it doesn't look valid.
     if (function_rva == 0 && sym_name.empty())
       continue;
     symbol.GetAddressRef() =
         Address(m_coff_header_opt.image_base + function_rva, sect_list);