[PATCH] D69671: [llvm-readobj] Change errors to warnings for symbol section name dumping

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Forgot to submit this inline comment.

Also, I discovered I'd missed a couple of yaml2obj tests that were testing for errors instead of warnings. I've fixed these and will commit directly without updating the diff, as they are trivial changes.



================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:562
+template <class ELFT>
+void DumpStyle<ELFT>::reportUniqueWarning(Error Err) const {
+  handleAllErrors(std::move(Err), [&](const ErrorInfoBase &EI) {
----------------
grimar wrote:
> jhenderson wrote:
> > grimar wrote:
> > > Our warnings are always unique. Should it be just `reportWarning` (it is much more common name for reporting)?
> > At the moment, nothing really forces our warnings to be unique. The llvm-readobj.h `reportWarning` function does not do any uniquifying, so if the same warning could be reported from two different places, it isn't unique. I think one such example where it isn't is `getSymbolForReloc`. There are others too in the stack size, addrsig and note printing code. Probably we could change all of these to call the new function.
> > 
> > A concern with calling the new function `reportWarning` is that it could easily clash with the `reportWarning` in llvm-readobj.h, and people could end up calling the wrong one. I'm not sure of a good way to resolve this, as `reportUniqueWarning` could be easily missed in the future. What do you think?
> > At the moment, nothing really forces our warnings to be unique. 
> 
> Yeah, I meant ones reported with the use of `WarningHandler`. I've not realized it is almost never used still.
> 
> > I'm not sure of a good way to resolve this, as reportUniqueWarning could be easily missed in the future. 
> 
> So currently we have a global `reportWarning(Error Err, StringRef Input)` and this new `reportUniqueWarning(Error Err)`.
> 
> I am thinking about the following steps:
> 1) We can switch all places that use `reportWarning(Error Err, StringRef Input)` to use `reportUniqueWarning(Error Err)` where possible/reasonable/easy (sometimes seems it is not that simple atm).
> It looks like this might cover most of the places => will make normal for warnings to be unique.
> 
> 2) We can rename `reportWarning(Error Err, StringRef Input)` to `reportNonUniqueWarning(Error Err, StringRef Input)` to emphasize there is something unusual/wrong with it. It is actually is, right? We probably want to report only unique warnings everywhere or almost everywhere.
> 
> 3) And then we can rename `reportUniqueWarning` to just `reportWarning`, it will suggest it to be our normal warning reporting API.
> 
> How does it sound?
> 
> Anyways seems for now it is OK to keep the name of `reportUniqueWarning` as is, as the problem is a bit more complex as I supposed initially.
That sounds like a reasonable strategy. At the moment, the majority of usages of `reportWarning` are in ELFDumper, and I suspect that they can all be migrated over (some are unique due to the code structure so don't necessarily need to be, but I think there is no harm in doing so). We might want to consider a strategy for making it always unique, but that would require larger code changes. In general, I think warnings should be unique. There may be cases where the warnings are warning about the same issue in different parts of an object, and should be reported separately, but really the problem there is the lack of sufficient context to the warning.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69671