[PATCH] Added new functionality to LLVM C API to use DiagnosticInfo to handle errors

Mon Apr 21 23:55:21 PDT 2014

Quentin Colombet wrote:
> On Apr 4, 2014, at 11:18 AM, Tom Stellard <tom at stellard.net
> <mailto:tom at stellard.net>> wrote:
>
>> On Fri, Apr 04, 2014 at 10:59:11AM -0700, Eric Christopher wrote:
>>> I'd very much prefer not to expose the severity for now. This is a
>>> fairly well thought out, but very new interface and we guarantee
>>> stability of the C interface so we'd be locking the current severity
>>> interface down.
>>>
>>> I'm not, honestly, a huge fan of opening this up to the C API in
>>> general - is there a good reason to do so?
>>>
>>
>> We would like to use this in Mesa so we can report backend compiler
>> errors to the graphics drivers. The most important use case is reporting
>> when the verifier detects an illegal instruction, because executing
>> illegal instructions has a good chance of locking up the system.
>>
>> We'd also like to query the severity level, but we really only care
>> about Errors
>> and not the others. In fact we probably could just use the severity
>> and not
>> worry about the error message. What if we limited the C API to just:
>>
>> LLVMContextSetDiagnosticHandler(LLVMContextRef C,
>> LLVMDiagnosticHandler Handler,
>> void *DiagnosticContext);
>> /// @returns true if DiagnosticSeverity is DS_Error and false otherwise.
>> bool LLVMDiagInfoHasError(LLVMDiagnosticInfoRef DI);
>>
>> Would this work?
>
> FWIW, the C API of lto has the severity thing.
>
> The diagnostic handler is slightly different too and deal with string
> and severity:
> /**
> * Diagnostic severity.
> *
> * \since LTO_API_VERSION=7
> */
> typedef enum {
> LTO_DS_ERROR = 0,
> LTO_DS_WARNING = 1,
> LTO_DS_REMARK = 3, // Added in LTO_API_VERSION=10.
> LTO_DS_NOTE = 2
> } lto_codegen_diagnostic_severity_t;

Low level comments:

Multiply these by 10 so that we can add new levels in between.

What's the ABI guarantee on an enum? Is this going to be an int? A char? 
I think it would be smarter to pick an underlying type and use that 
instead of letting implementations choose an underlying type (which I 
think is what the std says now).

High level comments:

I haven't actually reviewed this API or thought about whether it belongs 
in here, but it seems unavoidable now that we have backend "remarks". My 
initial reaction is that I would rather remove backend remarks.

> /**
> * Diagnostic handler type.
> * \p severity defines the severity.
> * \p diag is the actual diagnostic.
> * The diagnostic is not prefixed by any of severity keyword, e.g.,
> 'error: '.
> * \p ctxt is used to pass the context set with the diagnostic handler.
> *
> * \since LTO_API_VERSION=7
> */
> typedef void (*lto_diagnostic_handler_t)(
> lto_codegen_diagnostic_severity_t severity, const char *diag, void *ctxt);
>
> -Quentin
>
>>
>> -Tom
>>
>>>
>>> On Fri, Apr 4, 2014 at 10:39 AM, Quentin Colombet
>>> <qcolombet at apple.com <mailto:qcolombet at apple.com>> wrote:
>>>> Hi Darren,
>>>>
>>>> This LGTM with two comments:
>>>> - LLVMPrintDiagInfoToString should state that the string has to be freed
>>>> (see inline comment).
>>>> - Should we provide an API to expose the severity as well? What
>>>> would do the
>>>> users of LLVMPrintDiagInfoToString if we do not?
>>>>
>>>> Note that I'm not super familiar with the C API, so you may want to
>>>> wait for
>>>> another review.
>>>>
>>>> Thanks,
>>>> -Quentin
>>>>
>>>> On Apr 2, 2014, at 8:32 PM, Tom Stellard <tom at stellard.net
>>>> <mailto:tom at stellard.net>> wrote:
>>>>
>>>> From: Darren Powell <darren.powell at amd.com
>>>> <mailto:darren.powell at amd.com>>
>>>>
>>>> ---
>>>> include/llvm-c/Core.h | 20 ++++++++++++++++++++
>>>> include/llvm/IR/DiagnosticInfo.h | 3 +++
>>>> lib/IR/Core.cpp | 20 ++++++++++++++++++++
>>>> 3 files changed, 43 insertions(+)
>>>>
>>>> diff --git a/include/llvm-c/Core.h b/include/llvm-c/Core.h
>>>> index 50c5e3a..17367ff 100644
>>>> --- a/include/llvm-c/Core.h
>>>> +++ b/include/llvm-c/Core.h
>>>> @@ -124,6 +124,12 @@ typedef struct LLVMOpaquePassRegistry
>>>> *LLVMPassRegistryRef;
>>>> * @see llvm::Use */
>>>> typedef struct LLVMOpaqueUse *LLVMUseRef;
>>>>
>>>> +
>>>> +/**
>>>> + * @see llvm::DiagnosticInfo
>>>> + */
>>>> +typedef struct LLVMOpaqueDiagnosticInfo *LLVMDiagnosticInfoRef;
>>>> +
>>>> typedef enum {
>>>> LLVMZExtAttribute = 1<<0,
>>>> LLVMSExtAttribute = 1<<1,
>>>> @@ -453,6 +459,8 @@ void LLVMEnablePrettyStackTrace(void);
>>>> * @{
>>>> */
>>>>
>>>> +typedef void (*LLVMDiagnosticHandler)(LLVMDiagnosticInfoRef, void *);
>>>> +
>>>> /**
>>>> * Create a new context.
>>>> *
>>>> @@ -467,6 +475,13 @@ LLVMContextRef LLVMContextCreate(void);
>>>> LLVMContextRef LLVMGetGlobalContext(void);
>>>>
>>>> /**
>>>> + * Set the diagnostic handler for this context.
>>>> + */
>>>> +void LLVMContextSetDiagnosticHandler(LLVMContextRef C,
>>>> + LLVMDiagnosticHandler Handler,
>>>> + void *DiagnosticContext);
>>>> +
>>>> +/**
>>>> * Destroy a context instance.
>>>> *
>>>> * This should be called for every call to LLVMContextCreate() or memory
>>>> @@ -474,6 +489,11 @@ LLVMContextRef LLVMGetGlobalContext(void);
>>>> */
>>>> void LLVMContextDispose(LLVMContextRef C);
>>>>
>>>> +/**
>>>> + * Extract string description from DiagnosticInfo.
>>>>
>>>>
>>>> Add a comment here saying that the string should be freed using
>>>> LLVMDisposeMessage.
>>>>
>>>> + */
>>>> +char *LLVMPrintDiagInfoToString(LLVMDiagnosticInfoRef DI);
>>>> +
>>>> unsigned LLVMGetMDKindIDInContext(LLVMContextRef C, const char* Name,
>>>> unsigned SLen);
>>>> unsigned LLVMGetMDKindID(const char* Name, unsigned SLen);
>>>> diff --git a/include/llvm/IR/DiagnosticInfo.h
>>>> b/include/llvm/IR/DiagnosticInfo.h
>>>> index de2a9d8..5fd6636 100644
>>>> --- a/include/llvm/IR/DiagnosticInfo.h
>>>> +++ b/include/llvm/IR/DiagnosticInfo.h
>>>> @@ -193,6 +193,9 @@ public:
>>>> }
>>>> };
>>>>
>>>> +// Create wrappers for C Binding types (see CBindingWrapping.h).
>>>> +DEFINE_SIMPLE_CONVERSION_FUNCTIONS(DiagnosticInfo,
>>>> LLVMDiagnosticInfoRef)
>>>> +
>>>> } // End namespace llvm
>>>>
>>>> #endif
>>>> diff --git a/lib/IR/Core.cpp b/lib/IR/Core.cpp
>>>> index 4f3d8b1..e3b70c7 100644
>>>> --- a/lib/IR/Core.cpp
>>>> +++ b/lib/IR/Core.cpp
>>>> @@ -17,6 +17,8 @@
>>>> #include "llvm/IR/Attributes.h"
>>>> #include "llvm/IR/CallSite.h"
>>>> #include "llvm/IR/Constants.h"
>>>> +#include "llvm/IR/DiagnosticInfo.h"
>>>> +#include "llvm/IR/DiagnosticPrinter.h"
>>>> #include "llvm/IR/DerivedTypes.h"
>>>> #include "llvm/IR/GlobalAlias.h"
>>>> #include "llvm/IR/GlobalVariable.h"
>>>> @@ -76,6 +78,14 @@ LLVMContextRef LLVMGetGlobalContext() {
>>>> return wrap(&getGlobalContext());
>>>> }
>>>>
>>>> +void LLVMContextSetDiagnosticHandler(LLVMContextRef C,
>>>> + LLVMDiagnosticHandler Handler,
>>>> + void *DiagnosticContext) {
>>>> + unwrap(C)->setDiagnosticHandler(
>>>> + LLVM_EXTENSION
>>>> reinterpret_cast<LLVMContext::DiagnosticHandlerTy>(Handler),
>>>> + DiagnosticContext);
>>>> +}
>>>> +
>>>> void LLVMContextDispose(LLVMContextRef C) {
>>>> delete unwrap(C);
>>>> }
>>>> @@ -89,6 +99,16 @@ unsigned LLVMGetMDKindID(const char* Name,
>>>> unsigned SLen)
>>>> {
>>>> return LLVMGetMDKindIDInContext(LLVMGetGlobalContext(), Name, SLen);
>>>> }
>>>>
>>>> +char *LLVMPrintDiagInfoToString(LLVMDiagnosticInfoRef DI) {
>>>> + std::string MsgStorage;
>>>> + raw_string_ostream Stream(MsgStorage);
>>>> + DiagnosticPrinterRawOStream DP(Stream);
>>>> +
>>>> + unwrap(DI)->print(DP);
>>>> + Stream.flush();
>>>> +
>>>> + return strdup(MsgStorage.c_str());
>>>> +}
>>>>
>>>> /*===-- Operations on modules
>>>> ---------------------------------------------===*/
>>>>
>>>> --
>>>> 1.8.3.2
>>>>
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits