[PATCH] Add diagnostic capabilities in LLVM (frontend part)

[Quentin Colombet]

Hi Hal,

Thanks for you feedbacks.

On Dec 17, 2013, at 3:53 PM, Hal Finkel <hfinkel at anl.gov> wrote:

> Hi Quentin,
> 
> Any reason you did not use phabricator for this patch?
I thought it was trivial enough not to require a phabricator.

> 
> +def BackendInlineAsm : DiagGroup<"backend-inline-asm">;
> 
> +def BackendStackSize : DiagGroup<"backend-stack-size">;
> 
> +def BackendPlugin : DiagGroup<"backend-plugin">;
> 
> Are these group names user-visible?
I think they are. At least, I get this name when a warning is issued:
warning: stack size exceeded (168) in main [-Wbackend-stack-size]
1 warning generated.

> If they are, do we want "backend" in the name?
I do not have any strong opinion on that.
This prefix makes it clear that these diagnostics are issued by the backend. I thought it may be useful to have a naming convention for those.

> 
> +  // FIXME: We should demangle the function name.
> 
> +  // FIXME: Is there a way to get a location for that function?
> 
> +  FullSourceLoc Loc;
> 
> +  Diags.Report(Loc, diag::warn_fe_backend_stack_size)
> 
> +      << D.getStackSize() << D.getFunction().getName();
> 
> +  return true;
> 
> +}
> 
> +
> 
> We should figure this out ;)
This was part of the missing points, so if we can ,sure!
The question is should we hold the patch on that... I really do not know. At first glance, it does not seem easy to demangle function name, but I am not familiar with clang’s internal and I may have miss something obvious.

> -- Furthermore, since you put in the work to have the diagnostic printers, we should use that to handle this stack-size warning instead of using this special class.
I disagree. In my opinion the diagnostic printer should be reserved for unknown diagnostic (what we called plugins). Indeed, as soon as a diagnostic is statically known, I prefer to have a special case in the frontend so what the localization burden is in there (e.g., for the stack size warning the message string is in the related td file).

> 
> Looking through CodeGenModule.cpp, it does not look like there is such a map currently, although it looks like we could maintain such a map in the CodeGenModule::GetOrCreateLLVMFunction function.
You mean to demangle function name?

-Quentin

> 
> -Hal
> 
> ----- Original Message -----
>> From: "Quentin Colombet" <qcolombet at apple.com>
>> To: "Dmitri Gribenko" <gribozavr at gmail.com>
>> Cc: "llvm cfe" <cfe-commits at cs.uiuc.edu>
>> Sent: Tuesday, December 17, 2013 5:03:36 PM
>> Subject: Re: [PATCH] Add diagnostic capabilities in LLVM (frontend part)
>> 
>> 
>> 
>> Hi Dimitri,
>> 
>> Thanks for the quick reply!
>> 
>> New patch attached.
>> 
>> On Dec 17, 2013, at 11:58 AM, Dmitri Gribenko <gribozavr at gmail.com>
>> wrote:
>> 
>>> On Tue, Dec 17, 2013 at 11:22 AM, Quentin Colombet
>>> <qcolombet at apple.com> wrote:
>>>> - Tests.
>>>> I made some tests locally, but the patch does not add any test
>>>> case. The
>>>> question is how can we add tests that require both a backend and a
>>>> frontend
>>>> in either the frontend or the backend?
>>> 
>>> A lot of tests in Clang have "REQUIRES: x86-registered-target" --
>>> please take a look at them.
>> Thanks, using your advice, I have added a test in
>> test/Frontend/backend-diagnostic.c.
>> 
>>> 
>>> About the patch:
>>> 
>>> 80 columns.
>> Oh sorry about that, I was sure I had run clang-format… apparently
>> not!
>> Fixed.
>> 
>>> 
>>> +/// DiagnosticHandler2 - This function is invoked when the backend
>>> needs
>>> +/// to report something to the user.
>>> 
>>> Please don't duplicate function name in comments.
>> Fixed.
>> 
>>> 
>>> +/// The \p DI interface provides two basic information:
>>> +/// getKind() helps to determines what it is reporting and
>>> +/// getSeverity() decribes how bad this is.
>>> 
>>> This does not really belong to the documentation for
>>> DiagnosticHandler2(), I think.
>> Removed.
>> 
>>> 
>>> +#define SeveritySwitch(Severity, GroupName, DiagID)
>>> \
>>> + switch (Severity) {
>>> \
>>> 
>>> Please wrap this in do {...} while(false), so that the trailing
>>> semicolon in the macro invocation gets correctly attached to the
>>> expansion.
>> Done.
>> 
>>> 
>>> Also, this macro could have a better name -- maybe ComputeSeverity?
>> Renamed into ComputeDiagID.
>> 
>> Thanks,
>> -Quentin
>> 
>> 
>> 
>> 
>>> 
>>> Dmitri
>>> 
>>> --
>>> main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
>>> (j){printf("%d\n",i);}}} /*Dmitri Gribenko <gribozavr at gmail.com>*/
>> 
>> 
>> 
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>> 
> 
> -- 
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131217/089b5e27/attachment.html>