[PATCH] Add warning capabilities in LLVM (backend part), Take 2

[Hal Finkel]

----- Original Message -----
> From: "Quentin Colombet" <qcolombet at apple.com>
> To: "Renato Golin" <renato.golin at linaro.org>
> Cc: "Hal Finkel" <hfinkel at anl.gov>, "David Blaikie" <dblaikie at gmail.com>, "Chandler Carruth" <chandlerc at gmail.com>,
> "Eric Christopher" <echristo at gmail.com>, "llvm-commits" <llvm-commits at cs.uiuc.edu>,
> reviews+D2376+public+8c083b1648e3e7b6 at llvm-reviews.chandlerc.com
> Sent: Wednesday, December 11, 2013 12:20:28 PM
> Subject: Re: [PATCH] Add warning capabilities in LLVM (backend part), Take 2
> 
> Hi Renato,
> 
> On Dec 11, 2013, at 5:06 AM, Renato Golin <renato.golin at linaro.org>
> wrote:
> 
> > On 11 December 2013 12:52, Hal Finkel <hfinkel at anl.gov> wrote:
> >>>> For what do you imagine that DiagnosticInfoOther will be used?
> >>> I was thinking plugins.
> > 
> > I think it should be easy for third-party implementations (not-yet
> > or
> > never-to-be merged code) to have their own non-clashing handlers,
> > which ends up as deriving from the base class only on your tree and
> > make use of it only by your code.
> If everyone agrees we do not need the DiagnosticInfoOther, I am fine
> removing it.
> However, beyond the plugin case, I thought it would have been a good
> class to experiment things (like vectorizer information on why
> something has not been optimized using a DS_Note) or to report a
> richer message than InlineAsm without creating a specific subclass
> for each pattern.

Understood, but I don't want to encourage this behavior ;) I'd rather we leave this out unless it is specifically requested.

> 
> > 
> > 
> >> 1. enum DiagnosticKind should end with DK_FirstPluginKind
> > 
> > This is ok, because plugins can use/reuse the same enum values, but
> > it
> > makes it hard for keeping trees in sync. Worst still, if you use
> > two
> > plugins that happen to use the same enum values, we'll only find
> > out
> > if they both run on the same binary and report an error.
> > 
> > Open source plugins can have their values added to the main tree,
> > closed source ones cannot.
> I think Hal’s suggestion is perfectly fine. The diagnostic ID in the
> plugins will be set at runtime using the
> getNextAvailablePluginDiagnosticKind(). Therefore, I do not see any
> problem here, except maybe thread safety when updating the next
> available diagnostic kind.

Yes; this is why I suggested that it be a function (and not just some global static variable): we can provide locking if necessary (in reality, using an atomic update is probably sufficient).

> 
> > 
> > 
> >> 2. Add a global function: int
> >> getNextAvailablePluginDiagnosticKind() which returns an
> >> incremented static variable (which starts with
> >> DK_FirstPluginKind).
> > 
> > I think we can start simple, and add a DK_PluginKind and make all
> > plugins differentiate themselves in the error message,
> The only difference I see here with the current DK_Other, is the name
> in the enum :).
> 
> > or even having
> > a customized payload for the C-API which passes a PDK_Kind (Plugin
> > Diagnostic Kind). But I think this needs more discussion.
> For now, I think Hal’s suggestion is a good start. That said, I was
> not looking in addressing the problem adding new diagnostics at
> runtime. Thus, we may need another thread to discuss that.
> I do not think that was block the current proposal.

I agree; let's just address the in-tree uses that we currently envision, and not add any non-trivial code to support plugins until we discuss that design further.

Thanks again,
Hal

> 
> What do you think?
> 
> Cheers,
> Quentin
> > 
> > cheers,
> > --renato
> 
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory