[cfe-commits] [PATCH] Clang support for MemorySanitizer

[Richard Smith]

On Tue, Dec 4, 2012 at 3:04 AM, Evgeniy Stepanov <eugenis at google.com> wrote:

> Nice. Thanks for the pointer.
> Does the attached patch look good?
>

Looks fine. Presumably a test needs updating too?


> On Tue, Dec 4, 2012 at 2:00 AM, Richard Smith <richard at metafoo.co.uk>wrote:
>
>> On Fri, Nov 30, 2012 at 3:01 AM, Chandler Carruth <chandlerc at gmail.com>wrote:
>>
>>>
>>>
>>> ================
>>> Comment at:
>>> llvm/tools/clang/include/clang/Basic/DiagnosticDriverKinds.td:104-107
>>> @@ -103,2 +103,6 @@
>>>    "AddressSanitizer on Android requires '-pie'">;
>>> +def err_drv_tsan_requires_pie : Error<
>>> +  "ThreadSanitizer requires '-pie'">;
>>> +def err_drv_msan_requires_pie : Error<
>>> +  "MemorySanitizer requires '-pie'">;
>>>  def err_drv_unknown_objc_runtime : Error<
>>> ----------------
>>> Evgeniy Stepanov wrote:
>>> > Dmitri Gribenko wrote:
>>> > > Chandler Carruth wrote:
>>> > > > You don't need two diagnostics:
>>> > > >
>>> > > >   def err_drv_sanitizer_requires_pie : Error<
>>> > > >     "%select{thread|memory}0 sanitizer requires the '-pie'
>>> option">;
>>> > > I think it is better to use two diagnostics since %select would
>>> require magic numbers in the source.
>>> > I also prefer two diagnosticts.
>>> >
>>> > Btw, %select{a|b}0 evaluates to "b" if true, and "a" if false. I find
>>> this _really_ weird.
>>> >
>>> I don't like that pattern any more than you do, but Clang's diagnostics
>>> use it *extensively*, adding a comment next to the magic number indicating
>>> which is which.
>>>
>>> I also dislike the duplicated text intensely.
>>>
>>> A good solution would be something like this:
>>>
>>>   def err_drv_sanitizer_requires_pie : Error<
>>>     "%select{thread|memory}0 sanitizer requires the '-pie' option">,
>>>     SelectNames<0, { SDK_Thread, SDK_Memory }>;
>>>
>>> Where this automatically teaches the diagnostic engine that select #0
>>> should accept a specific enum rather than an unsigned, and defines that
>>> enum in the diag namespace with the given enumerators.
>>>
>>> But this would be a huge (good) change and shouldn't be in this patch. I
>>> think I mildly prefer consistency with other diagnostics until we get
>>> something like this.
>>
>>
>> We already have diag::err_drv_argument_only_allowed_with for indicating
>> that one argument requires another.
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121204/ab0e01d0/attachment.html>