[Lldb-commits] [PATCH] D147669: PECOFF: enforce move semantics and consume errors properly

2023-04-07 Thread Saleem Abdulrasool via Phabricator via lldb-commits 
compnerd added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147669/new/

https://reviews.llvm.org/D147669

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D147669: PECOFF: enforce move semantics and consume errors properly

2023-04-07 Thread Saleem Abdulrasool via Phabricator via lldb-commits 
compnerd added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147669/new/

https://reviews.llvm.org/D147669

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D147669: PECOFF: enforce move semantics and consume errors properly

2023-04-07 Thread Alvin Wong via Phabricator via lldb-commits 
alvinhochun added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147669/new/

https://reviews.llvm.org/D147669

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D147669: PECOFF: enforce move semantics and consume errors properly

2023-04-06 Thread Stefan Gränitz via Phabricator via lldb-commits 
sgraenitz requested changes to this revision.
sgraenitz added a comment.
This revision now requires changes to proceed.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147669/new/

https://reviews.llvm.org/D147669

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D147669: PECOFF: enforce move semantics and consume errors properly

2023-04-06 Thread Alex Langford via Phabricator via lldb-commits 
bulbazord added inline comments.



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:869-873
   LLDB_LOG(log,
"ObjectFilePECOFF::AppendFromExportTable - failed to get export 
"
"table entry name: {0}",
llvm::fmt_consume(std::move(err)));
+  llvm::consumeError(std::move(err));

compnerd wrote:
> compnerd wrote:
> > compnerd wrote:
> > > bulbazord wrote:
> > > > 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 :/
> > > Yeah ... I was worried about that.
> > 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
> > ```
> https://godbolt.org/z/nj4r7K8hb
> 
> UBSAN also seems to indicate it is permissible.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147669/new/

https://reviews.llvm.org/D147669

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D147669: PECOFF: enforce move semantics and consume errors properly

2023-04-06 Thread Saleem Abdulrasool via Phabricator via lldb-commits 
compnerd added inline comments.



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:869-873
   LLDB_LOG(log,
"ObjectFilePECOFF::AppendFromExportTable - failed to get export 
"
"table entry name: {0}",
llvm::fmt_consume(std::move(err)));
+  llvm::consumeError(std::move(err));

compnerd wrote:
> compnerd wrote:
> > bulbazord wrote:
> > > 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 
> > > :/
> > Yeah ... I was worried about that.
> 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
> ```
https://godbolt.org/z/nj4r7K8hb

UBSAN also seems to indicate it is permissible.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147669/new/

https://reviews.llvm.org/D147669

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D147669: PECOFF: enforce move semantics and consume errors properly

2023-04-05 Thread Saleem Abdulrasool via Phabricator via lldb-commits 
compnerd added inline comments.



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:869-873
   LLDB_LOG(log,
"ObjectFilePECOFF::AppendFromExportTable - failed to get export 
"
"table entry name: {0}",
llvm::fmt_consume(std::move(err)));
+  llvm::consumeError(std::move(err));

compnerd wrote:
> bulbazord wrote:
> > 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 :/
> Yeah ... I was worried about that.
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
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147669/new/

https://reviews.llvm.org/D147669

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D147669: PECOFF: enforce move semantics and consume errors properly

2023-04-05 Thread Saleem Abdulrasool via Phabricator via lldb-commits 
compnerd added inline comments.



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:869-873
   LLDB_LOG(log,
"ObjectFilePECOFF::AppendFromExportTable - failed to get export 
"
"table entry name: {0}",
llvm::fmt_consume(std::move(err)));
+  llvm::consumeError(std::move(err));

bulbazord wrote:
> 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 :/
Yeah ... I was worried about that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147669/new/

https://reviews.llvm.org/D147669

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D147669: PECOFF: enforce move semantics and consume errors properly

2023-04-05 Thread Alex Langford via Phabricator via lldb-commits 
bulbazord added inline comments.



Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:869-873
   LLDB_LOG(log,
"ObjectFilePECOFF::AppendFromExportTable - failed to get export 
"
"table entry name: {0}",
llvm::fmt_consume(std::move(err)));
+  llvm::consumeError(std::move(err));

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 :/


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147669/new/

https://reviews.llvm.org/D147669

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D147669: PECOFF: enforce move semantics and consume errors properly

2023-04-05 Thread Saleem Abdulrasool via Phabricator via lldb-commits 
compnerd created this revision.
compnerd added reviewers: bulbazord, sgraenitz, labath.
compnerd added a project: LLDB.
Herald added a subscriber: JDevlieghere.
Herald added a project: All.
compnerd requested review of this revision.

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, which resulted in
a check failure under `LLVM_ENABLE_ABI_BREAKING_CHANGES`.  The error
path is meant to log and continue on the failure but would instead abort
when built with assertions.

Secondary to the assignment cast check, 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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D147669

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


Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -865,11 +865,12 @@
   // 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)) {
+if (auto err = std::move(entry.getSymbolName(sym_name))) {
   LLDB_LOG(log,
"ObjectFilePECOFF::AppendFromExportTable - failed to get export 
"
"table entry name: {0}",
llvm::fmt_consume(std::move(err)));
+  llvm::consumeError(std::move(err));
   continue;
 }
 Symbol symbol;
@@ -886,11 +887,12 @@
   // 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)) {
+  if (auto err = std::move(entry.getForwardTo(forwarder_name))) {
 LLDB_LOG(log,
  "ObjectFilePECOFF::AppendFromExportTable - failed to get "
  "forwarder name of forwarder export '{0}': {1}",
  sym_name, llvm::fmt_consume(std::move(err)));
+llvm::consumeError(std::move(err));
 continue;
   }
   llvm::SmallString<256> new_name = 
{symbol.GetDisplayName().GetStringRef(),
@@ -901,11 +903,12 @@
 }
 
 uint32_t function_rva;
-if (auto err = entry.getExportRVA(function_rva)) {
+if (auto err = std::move(entry.getExportRVA(function_rva))) {
   LLDB_LOG(log,
"ObjectFilePECOFF::AppendFromExportTable - failed to get "
"address of export entry '{0}': {1}",
sym_name, llvm::fmt_consume(std::move(err)));
+  llvm::consumeError(std::move(err));
   continue;
 }
 // Skip the symbol if it doesn't look valid.


Index: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
===
--- lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
+++ lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
@@ -865,11 +865,12 @@
   // 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)) {
+if (auto err = std::move(entry.getSymbolName(sym_name))) {
   LLDB_LOG(log,
"ObjectFilePECOFF::AppendFromExportTable - failed to get export "
"table entry name: {0}",
llvm::fmt_consume(std::move(err)));
+  llvm::consumeError(std::move(err));
   continue;
 }
 Symbol symbol;
@@ -886,11 +887,12 @@
   // 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)) {
+  if (auto err = std::move(entry.getForwardTo(forwarder_name))) {
 LLDB_LOG(log,
  "ObjectFilePECOFF::AppendFromExportTable - failed to get "
  "forwarder name of forwarder export '{0}': {1}",
  sym_name, llvm::fmt_consume(std::move(err)));
+llvm::consumeError(std::move(err));
 continue;
   }
   llvm::SmallString<256> new_name = {symbol.GetDisplayName().GetStringRef(),
@@ -901,11 +903,12 @@
 }
 
 uint32_t function_rva;
-if (auto err = entry.getExportRVA(function_rva)) {
+if (auto err = std::move(entry.getExportRVA(function_rva))) {
   LLDB_LOG(log,
"ObjectFilePECOFF::AppendFromExportTable - failed to get "
"address of export entry '{0}': {1}",
sym_name