[PATCH] D120679: [Object] [COFF] Improve error messages

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added a comment.

In D120679#3350720 <https://reviews.llvm.org/D120679#3350720>, @jhenderson wrote:

> Thanks for the improvement. Whilst I get what @rnk's saying about testing, I've learnt from past experience that if an error message is not tested, there's a non-trivial chance that the message is garbled in some way, e.g. due to incorrect format strings, or using wrong parameters. I'd personally prefer seeing a test for each case that has the additional new context (including, where possible, using the actual values passed in, rather than regexes), at least if there's already testing for the error path.

While I agree that it would be ideal to have tests for every single case, that’s way out of scope for what I meant to spend effort on here. If that’s required, I’ll just drop this patch - that’s not what I set out to do.

I’m not changing any error handling part, I’m just adding unique strings to all textless cases of `object_error::parse_failed` in the file, so that if such a message gets printed, it can be mapped back to the code unambiguously. And we happen to have one test that triggers one of the changed messages, so we actually do get the formatting tested.

Some of the errors might even be impossible to trigger, if it’s just an internal consistency check that is guarded by an outer condition too. I’m not planning on investigating if it’s possible to trigger them all, and how to (many of them might require handcrafting broken binaries), I’m just adding messages.



================
Comment at: llvm/lib/Object/COFFObjectFile.cpp:488
+    return createStringError(object_error::parse_failed,
+                             "RVA %" PRIx32 " for %s not found", Addr,
+                             ErrorContext);
----------------
jhenderson wrote:
> Here and below, do you want the "0x" prefix?
Sure, that’s probably better.


================
Comment at: llvm/lib/Object/COFFObjectFile.cpp:490
+                             ErrorContext);
+  else
+    return createStringError(object_error::parse_failed,
----------------
jhenderson wrote:
> No need for `else` after `return`.
Ok, sure.


================
Comment at: llvm/test/tools/llvm-readobj/COFF/tls-directory.test:107
 
-# BAD-TLS-RVA-ERR: error: '{{.*}}': Invalid data was encountered while parsing the file
+# BAD-TLS-RVA-ERR: error: '{{.*}}': RVA {{.*}} for TLS directory not found
 
----------------
jhenderson wrote:
> For the filename parameter, I'd consider passing in the filename as a FileCheck variable, although I acknowledge it didn't before, so am not too fussed by this.
> 
> For the RVA parameter, if you don't want to hard-code in the value, I'd use a FileCheck numeric variable to ensure that the value is actually an expected number. A possible style is in the inline edit, but you may want a tighter restriction etc.
Ok, I guess we can add the literal address, as it’s probably constant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120679