Specifying enums in TableGen for uses of %select

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

Repurposing the thread (hope the new name and recipients will be enough).

While I (generally) like the idea of this enum, I do worry a bit: I think that in a number of cases a single enum is shared between multiple diagnostics. It’s not a barrier to implementation, but something to account for => maybe this means that a new specific “class” should be specified in TableGen (giving both the type and enumerator list) and then linked in into the diagnostic.

I fail to see however how a “typed” approach could be used with the streaming interface used to fill in the diagnosis. Did I miss something ?

And in general, whilst the streaming “interface” of diagnostics is “easy”, it’s unfortunately error-prone. A more principled approach could be to generate a method for each diagnostic, and this method would have “proper” arguments. I am not sure this is tractable though, because of the huge number of diagnostics it might slow down compilation times considerably.

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

Repurposing the thread (hope the new name and recipients will be enough).

While I (generally) like the idea of this enum, I do worry a bit: I think
that in a number of cases a single enum is shared between multiple
diagnostics. It's not a barrier to implementation, but something to account
for => maybe this means that a new specific "class" should be specified in
TableGen (giving both the type and enumerator list) and then linked in into
the diagnostic.

I fail to see however how a "typed" approach could be used with the
streaming interface used to fill in the diagnosis. Did I miss something ?

Unfortunately, I don't know what particular implementation Chandler
had in mind, but here's something that we can do: we could assign a
unique value for each enumerator across all such enumerations and do
the appropriate bounds check in runtime.

And in general, whilst the streaming "interface" of diagnostics is "easy",
it's unfortunately error-prone. A more principled approach could be to
generate a method for each diagnostic, and this method would have "proper"
arguments. I am not sure this is tractable though, because of the huge
number of diagnostics it might slow down compilation times considerably.

And given that our diagnostics can essentially have a variable number
of arguments, but the number of arguments depends on some other
arguments' values ("%select{|%1}0"), this is even more challenging to
make completely safe.

Dmitri