[llvm] [Support] Add WrappedError class (PR #191706)

[James Henderson via llvm-commits]

jh7370 wrote:

> It feels a bit odd to produce error messages like "foo.o : :", especially when "foo.o :" is part of the error context. We usually structure output as Context: Error, which reads more naturally.

(I'm not sure if you noticed, but I forgot to escape my `<` and `>` characters when posting my previous comment, so my example was `foo.o: <some context>: <key failure reason>`. Hopefully that was somewhat obvious from the rest of my comments)
 
> If we instead use Error: Context, we would need an additional parameter to decide whether the context appears as a prefix or suffix. That feels like unnecessary complexity at this stage.

I'm not sure I follow exactly the statement here. I do agree that we want to avoid further parameters indicating whether the "context" is significant or not. I think this can be achieved by simply not using the `ContextualizedError` approach for cases where the additional "context" is important and should be considered significant for deduplication purposes.

> That said, representing the error as a structured Error object rather than a plain string is a good idea, since it allows us to preserve more detailed information. However, I have some concerns about how this interacts with the decorator pattern.
> 
> Take FileError as an example. It is essentially a wrapped error consisting of a filename and an inner error. The question is: should we assume that users want to deduplicate based on the filename? What if they instead want to deduplicate based on the inner error?

I don't think there is a case I can think of where I'd want to treat filenames as insignificant for error/warning deduplication. Consider this made-up example: I have two objects both with the exact same corrupted field in the ELF header, plus a third object that is fine. The warning message for one of the objects on its own might be: "warning: bad.o: parsing ELF header: field X was not valid" (where "bad.o" is the object name, "parsing ELF header" is the context, and "field X was not valid" is the key part of the error message). Now, imagine I wanted to dump the ELF header of all three objects. Something like:
```
$ llvm-readelf -h bad.o good.o bad2.o
```
If we only consider the "key part" to be significant we'd get the following situation:

1. User runs llvm-readelf.
2. llvm-readelf produces the error "warning: bad.o: parsing ELF header: field X was not valid"
3. User will assume that the warning only applies to bad.o, because the message isn't printed for any other object, so fixes only bad.o.
4. User reruns llvm-readelf and gets the same message, but for bad2.o. User is annoyed, because they could have fixed bad2.o at the same time as bad.o.

> This introduces ambiguity in how operator== should behave, since it’s unclear which part of the error should be considered for equality. But the log() function is fine for using decorator pattern.

Each type of `ErrorInfo` that supported this pattern would decide what the uniqueness behaviour should be for the stuff it controls. Typically this would be "yes/no for the stuff I add" then "let the wrapped classes decide for stuff they own". I think `FileError` should treat the object name as significant, `ContextualizedError` should treat the context as insignificant, and `StringError` should treat its message as significant. This will achieve the desired effect.

NB: we should be careful where we use `ContextualizedError`, as there are cases where the context IS significant, because adding the context shows the errors refer to different things. For example, imagine parsing a GNU archive member header. Some of the header fields are numbers, represented as text (so char '1' etc as opposed to byte 0x01). A typical message for failure to convert might be simply "'abc' is not a number". The call site might add the context about which field it is, producing "failed to parse GID: 'abc' is not a number". However, it's clear that if two fields happen to have the exact same problem (so e.g. we also had "failed to parse UID: 'abc' is not a number), we'd want a message for them both, because they are distinct fields that are broken (much like the object file names indicate distinct problems), so "failed to parse GID" and "failed to parse UID" would need to be part of the "key stuff" for the message. (Of course, we _could_ go down the path of making `ContextualizedError` optionally significant, and then just put together chains of `ContextaulizedError` etc to produce the final thing, with some pieces significant and others not, but that seems unnecessary at this point.

https://github.com/llvm/llvm-project/pull/191706