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

[Chandler Carruth]

================
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.

================
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<
----------------
Chandler Carruth wrote:
> 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.
The idea is to model an enumeration of selectors, not a bool... The value accepted by the diagnostic engine is technically an 'unsigned'.


http://llvm-reviews.chandlerc.com/D146