r351629 - Emit !callback metadata and introduce the callback attribute

[Doerfert, Johannes via cfe-commits]

Thanks for the clarifications. I'll fix all mentioned points asap.

________________________________
From: Eli Friedman <efriedma at quicinc.com>
Sent: Thursday, January 31, 2019 18:17
To: Doerfert, Johannes; Chandler Carruth; cfe-commits at lists.llvm.org
Cc: Raja Venkateswaran
Subject: RE: r351629 - Emit !callback metadata and introduce the callback attribute

(Comments inline.)

> -----Original Message-----
> From: cfe-commits <cfe-commits-bounces at lists.llvm.org> On Behalf Of
> Doerfert, Johannes Rudolf via cfe-commits
> Sent: Tuesday, January 22, 2019 9:29 AM
> To: Chandler Carruth <chandlerc at gmail.com>
> Cc: cfe-commits cfe <cfe-commits at lists.llvm.org>
> Subject: Re: r351629 - Emit !callback metadata and introduce the callback
> attribute
>
> > Another thing I notecide is that this code assumes the system has
> > `pthread.h` -- what about systems without it? I mean, you can disable the
> > test, but it seems bad to lose test coverage just because of that.
>
> So far, I disabled the test with a later addition which makes sure this
> test is only run under Linux. I'm unsure why we loose coverage because
> of that?

There isn't any way to safely disable the test without disabling it everywhere.  Specifically, the current version requires both that the host is Unix, and that the default target is the host.  Otherwise, the compiler might not be able to find and/or build pthread.h .

>
> > I would much prefer that you provide your own stub `pthread.h` in the
> > Inputs/... tree of the test suite and use that to test this in a portable
> > way.
>
> I do not completely follow but I'm open to improving the test. Basically
> I have to make sure the builtin recognition will trigger on the header
> file and the contained declaration. If we can somehow do this in a
> portable way I'm all for it. Is that how we test other builtin gnu extensions?

You don't need a header file; clang doesn't actually care where the builtin is declared.  You can just write "void pthread_create(void*,void*,void*(void*), void*);" or whatever directly in the C file.  It'll trigger a warning, but with your patch, there's no way to declare pthread_create without triggering a warning, so that hardly matters.

On a related note, code generation tests should use %clang_cc1.  Among other things, this avoids accidentally including headers from the host system.

-Eli

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190201/659e3c7a/attachment.html>