[PATCH] D9913: Mark libm builtins as such.

[hfinkel at anl.gov]

hfinkel added a subscriber: hfinkel.
hfinkel added a comment.

In http://reviews.llvm.org/D9913#192821, @rengolin wrote:

> In http://reviews.llvm.org/D9913#192741, @chatur01 wrote:
>
> > I wasn't sure if Clang would be interested in compatibility in this case. So I originally focused on the C99 floating point stuff that is part of a standard I could read. Do you think I should patch up the above missing bits too?
>
>
> That's my point. If this patch makes sense on its own, we should mark them all.


I agree.

> I have no idea how to test it, though.

> 

> > What Clang currently does is declare the type-generic builtins like this with an empty parameter list, which gives weird behaviour in both C and C++ when you try to redeclare them. I'm marking them up as library calls so that I can selectively handle just the builtins I need to in my patch in http://reviews.llvm.org/D9912.

> 

> 

> There are many odd things about this... First, Clang declaring it empty looks like a hack in Clang to "cross that bridge when we get there" kind of "solution". Second, marking them as library calls, when they're really builtin versions of library calls, is also a bit dodgy. Finally, Android has a bad reputation of re-implementing low-level libraries by calling higher-level libraries (for example, __aeabi_memcpy calling libc's memcpy), which is *also* shoddy.

> 

> I have no preference, but it would be good to be only as consistent as we need to be. Ie. not enable things we're not going to use just because it's similar to the ones we will use.

> 

> Since this smells like another hack to make Android work without really fixing Clang, I'd suggest moving it to the original patch (http://reviews.llvm.org/D9912) with a good comment about the reason we're doing this.

> 

> > I tried to mention that with my link in Phabricator, but I realize I've failed to make it clear.  There doesn't seem to be a good way of posting a logical patch set to Phab.

> 

> 

> There isn't. That's why I do a (git diff -U999 master > patch) every time and deal with patch-sets only locally. Gerrit seems to be a lot better on this, but once you have submitted a patch set, squashing and rebasing gets *a lot* worse, so I'm not fond of either way. Github is better.

> 

> > Aside from http://reviews.llvm.org/D9912 relying on this change, I also think it's fine to go in on its own, since these builtins should be marked up as "F", given that they're listed in the C99 standard. Do you agree?

> 

> 

> AFAICS, they're not listed in C99, only their counterparts without the __builtin_ are. IIUC, these builtins are the compiler trying to be smart about implementation details


I don't understand this part of the conversation. include/clang/Basic/Builtins.def says:

  //  F -> this is a libc/libm function with a '__builtin_' prefix added.
  //  f -> this is a libc/libm function without the '__builtin_' prefix. It can
  //       be followed by ':headername:' to state which header this function
  //       comes from.

this patch does not mark these with 'f', but with 'F', and they are indeed libm functions with __builtin_ added.

> , and that's why Android shouldn't be fiddling with them in the first place. But alas, that's the way of the Android. Whatever compiles, ship it.

> 

> My opinion is to move this into http://reviews.llvm.org/D9912 and only change the ones you really need, with a comment on why you're changing (maybe a FIXME?).

> 

> cheers,

> --renato



http://reviews.llvm.org/D9913