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

> On Jul 21, 2020, at 14:30, Chris Lattner via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> 
> 
>> On Jul 21, 2020, at 11:50 AM, Jonathan Roelofs via llvm-dev
<llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>>
wrote:
>> 
>> A question of preference came up in https://reviews.llvm.org/D83588
<https://reviews.llvm.org/D83588> as to whether we ought to prefer
emitting TableGen error messages with all of the information in a single
diagnostic, or whether it makes sense to split things in an error+note style as
seen in many clang diagnostics. TableGen doesn’t use a DiagnosticsEngine, so the
concept of a fatal note following an error is a bit new/foreign there, but
perhaps something that makes sense adding.
>> 
>> Is there any precedence here for other internal llvm tooling (outside
of clang)? What’s the general consensus here?
> 
> I think it makes a lot of sense to add this to TableGen.  It is a widely
used tool and the QoI for its error messages could use a lot of improvement!
There is a lot of room from improvement in several details of TableGen. 
However, methinks that changes towards such a goal shouldn't be sneaked in
small doses.  Then it just adds more inconsistent error reporting to an already
inconsistent reporting.  If one wants to improve the error reporting in
TableGen, let one take it on himself this project, apart from one's other
patches.

__ 
Evandro Menezes ◊ SiFive ◊ Austin, TX

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

> On Jul 21, 2020, at 1:46 PM, Evandro Menezes <evandro.menezes at
sifive.com> wrote:
> 
> 
> 
>> On Jul 21, 2020, at 14:30, Chris Lattner via llvm-dev <llvm-dev at
lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote:
>> 
>> 
>> 
>>> On Jul 21, 2020, at 11:50 AM, Jonathan Roelofs via llvm-dev
<llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>>
wrote:
>>> 
>>> A question of preference came up in https://reviews.llvm.org/D83588
<https://reviews.llvm.org/D83588> as to whether we ought to prefer
emitting TableGen error messages with all of the information in a single
diagnostic, or whether it makes sense to split things in an error+note style as
seen in many clang diagnostics. TableGen doesn’t use a DiagnosticsEngine, so the
concept of a fatal note following an error is a bit new/foreign there, but
perhaps something that makes sense adding.
>>> 
>>> Is there any precedence here for other internal llvm tooling
(outside of clang)? What’s the general consensus here?
>> 
>> I think it makes a lot of sense to add this to TableGen.  It is a
widely used tool and the QoI for its error messages could use a lot of
improvement!
> 
> There is a lot of room from improvement in several details of TableGen. 
However, methinks that changes towards such a goal shouldn't be sneaked in
small doses.  Then it just adds more inconsistent error reporting to an already
inconsistent reporting.
Sure, let’s talk about what that end goal should be! Can you give some other
examples of where these inconsistencies could be improved?
>  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.
> 
> __ 
> Evandro Menezes ◊ SiFive ◊ Austin, TX
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200721/4ac6c449/attachment.html>

> On Jul 21, 2020, at 15:46, Evandro Menezes via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> There is a lot of room from improvement in several details of TableGen. 
However, methinks that changes towards such a goal shouldn't be sneaked in
small doses.  Then it just adds more inconsistent error reporting to an already
inconsistent reporting.  If one wants to improve the error reporting in
TableGen, let one take it on himself this project, apart from one's other
patches.
Realistically, replacing all of TableGen’s error reporting in one shot is not
going to happen. I don’t see any reason to delay fixing any particular error
because all of the others are bad. For TableGen, I’m happy enough when there is
an actual error and not an assert.

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

Hi, Jonathan.
> On Jul 21, 2020, at 17:15, Jonathan Roelofs <jonathan_roelofs at
apple.com> wrote:
> 
> 
> 
>> On Jul 21, 2020, at 1:46 PM, Evandro Menezes <evandro.menezes at
sifive.com <mailto:evandro.menezes at sifive.com>> wrote:
>> 
>> 
>> 
>>> On Jul 21, 2020, at 14:30, Chris Lattner via llvm-dev <llvm-dev
at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote:
>>> 
>>> 
>>> 
>>>> On Jul 21, 2020, at 11:50 AM, Jonathan Roelofs via llvm-dev
<llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>>
wrote:
>>>> 
>>>> A question of preference came up in
https://reviews.llvm.org/D83588 <https://reviews.llvm.org/D83588> as to
whether we ought to prefer emitting TableGen error messages with all of the
information in a single diagnostic, or whether it makes sense to split things in
an error+note style as seen in many clang diagnostics. TableGen doesn’t use a
DiagnosticsEngine, so the concept of a fatal note following an error is a bit
new/foreign there, but perhaps something that makes sense adding.
>>>> 
>>>> Is there any precedence here for other internal llvm tooling
(outside of clang)? What’s the general consensus here?
>>> 
>>> I think it makes a lot of sense to add this to TableGen.  It is a
widely used tool and the QoI for its error messages could use a lot of
improvement!
>> 
>> There is a lot of room from improvement in several details of TableGen.
However, methinks that changes towards such a goal shouldn't be sneaked in
small doses.  Then it just adds more inconsistent error reporting to an already
inconsistent reporting.
> 
> 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

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.

Your turn,

__ 
Evandro Menezes ◊ SiFive ◊ Austin, TX

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