llvm dev - Jul 2020 - [RFC] Preferred error/note style across non-clang tools, e.g. tablegen

> On Jul 22, 2020, at 18:09, Jonathan Roelofs <jonathan_roelofs at
apple.com> wrote:
> 
> 
> 
>> On Jul 22, 2020, at 4:31 PM, Evandro Menezes <evandro.menezes at
sifive.com <mailto:evandro.menezes at sifive.com>> wrote:
>>> Sure, let’s talk about what that end goal should be! Can you give
some other examples of where these inconsistencies could be improved?
>> 
>> None specifically comes to the top of my mind, but they are legion,
which I suppose there's no disagreement about it.  I myself committed a few
small patches to improve error reporting as I came along them.  In many cases,
merely adding the file name or line number where the error was found.
>> 
>> Then again, as Matt mentioned, TableGen prefers to assert instead of
erring out, which is quite unhelpful.  In this sense, by all means, an
incremental improvement over an assert is more than incremental, but essential.
>> 
>>>>  If one wants to improve the error reporting in TableGen, let
one take it on himself this project, apart from one's other patches.
>>> 
>>> Is incremental progress not welcome? I understand pushback on
refactoring combined with functional changes, but that’s not at all what this
is. The entire purpose of that patch is to improve these diagnostics, making it
easier to do scheduler model work.
>> 
>> My concern is that without a definition of the intended style of the
error messages, as they are incrementally modified, they will be changed
inconsistently.
>> 
>> IMO, the error messages should provide some minimum information to be
useful:
>> - Offending file
>> - Offending line number
>> - Offending structure (e.g., resource type, instruction name, etc)
>> - Offense
> 
> Agreed. This is a good minimum.
> 
> Sometimes there are cases where the offense is a conflict between two
definitions, and in a lot of other tooling this is represented as a note that
points at the other definition. This helps because it gives you some context
around the two definitions in a way that appending the file + line number to the
error message doesn’t.
Understood.
>> I do not personally like the error and note, since in the patch above,
the note is always fatal, whereas I'd rather see what other errors there are
before it aborts.
> 
> There is plenty of prior-art even just in CodeGenSchedule for
error-and-non-fatal-notes (see collectRetireControlUnits,
collectLoadStoreQueueInfo). These new ones merely maintain existing behavior of
exiting after the diagnostic. Seems this objection is an objection to how it
already behaves, and not to the changes I’m proposing.
I may be missing something, but we'll discuss the patch in its review. 
However, unless an error results in a barrage of errors because something
crucial was broken, I'd rather see all error messages.  Sometimes, building
is tedious (e.g., batch system), so I'd rather be aware of all issues and
fix them before trying another build.

Thank you,

__ 
Evandro Menezes ◊ SiFive ◊ Austin, TX

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200724/b70811e3/attachment.html>

> On Jul 24, 2020, at 11:23 AM, Evandro Menezes <evandro.menezes at
sifive.com> wrote:
> 
>>> 
>>> I do not personally like the error and note, since in the patch
above, the note is always fatal, whereas I'd rather see what other errors
there are before it aborts.
>> 
>> There is plenty of prior-art even just in CodeGenSchedule for
error-and-non-fatal-notes (see collectRetireControlUnits,
collectLoadStoreQueueInfo). These new ones merely maintain existing behavior of
exiting after the diagnostic. Seems this objection is an objection to how it
already behaves, and not to the changes I’m proposing.
> 
> I may be missing something, but we'll discuss the patch in its review. 
However, unless an error results in a barrage of errors because something
crucial was broken, I'd rather see all error messages.  Sometimes, building
is tedious (e.g., batch system), so I'd rather be aware of all issues and
fix them before trying another build.
Maybe I missed something here: I thought the proposal was to add additional
notes to existing error messages, with the goal of clarifying the output and
improving the user experience.

I didn’t think the intention was to turn error messages into notes.  Did I get
that wrong?

Notes serve completely different role than errors: notes are an “add on” to a
warning or error, they should never be a primary diagnostic.

-Chris
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200724/0e120c5a/attachment.html>

> On Jul 24, 2020, at 2:03 PM, Chris Lattner <clattner at nondot.org>
wrote:
> 
> 
> 
>> On Jul 24, 2020, at 11:23 AM, Evandro Menezes <evandro.menezes at
sifive.com <mailto:evandro.menezes at sifive.com>> wrote:
>> 
>>>> 
>>>> I do not personally like the error and note, since in the patch
above, the note is always fatal, whereas I'd rather see what other errors
there are before it aborts.
>>> 
>>> There is plenty of prior-art even just in CodeGenSchedule for
error-and-non-fatal-notes (see collectRetireControlUnits,
collectLoadStoreQueueInfo). These new ones merely maintain existing behavior of
exiting after the diagnostic. Seems this objection is an objection to how it
already behaves, and not to the changes I’m proposing.
>> 
>> I may be missing something, but we'll discuss the patch in its
review.  However, unless an error results in a barrage of errors because
something crucial was broken, I'd rather see all error messages.  Sometimes,
building is tedious (e.g., batch system), so I'd rather be aware of all
issues and fix them before trying another build.
> 
> Maybe I missed something here: I thought the proposal was to add additional
notes to existing error messages, with the goal of clarifying the output and
improving the user experience.
> 
> I didn’t think the intention was to turn error messages into notes.  Did I
get that wrong?
The patch transforms a few `PrintFatalError()`s into `PrintError();
PrintFatalNote();`s.

Evandro is asking me to turn it instead into `PrintError(); PrintNote();`, which
I’m not confident is okay: this might invalidate some invariant and lead to the
tool crashing later. I’d rather not lump that in with my changes.


Jon
> 
> Notes serve completely different role than errors: notes are an “add on” to
a warning or error, they should never be a primary diagnostic.
> 
> -Chris
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200724/2fc45f07/attachment.html>