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

Tue Mar 1 06:13:08 PST 2022

jhenderson added a comment.

In D120679#3350849 <https://reviews.llvm.org/D120679#3350849>, @mstorsjo wrote:

> 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.

Just to be clear, I'm not suggesting adding new tests where they don't exist, only enhancing existing testing to be a little stricter (if there is such testing). If such testing doesn't exist, then what you've done is fine.

================
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

----------------
mstorsjo wrote:
> 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.
I'm mostly interested in getting rid of the naked wildcard regex, since it could hide dodgy content.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120679