[PATCH] Mark libm builtins as such.

[Charlie Turner]

On 23 June 2015 at 15:50, Renato Golin <renato.golin at linaro.org> wrote:
>
> That's my point. If this patch makes sense on its own, we should mark them all. I have no idea how to test it, though.
>

I would probably have to create a new suite of unit tests, ensuring
that for each ISO mode, only the correct set of library functions are
marked as isLibFunction(). I don't see any machinery in Clang that can
verify such things though, the builtins are lumped together.

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

And you lose the contextual advantage distinct commits as well. I
think I'd prefer to just be more explicit about relationships between
my Phab reviews. In hindsight it was a mistake of mine not to market
this patch as a distinct entity. Can we pretend that I did? :)

>> 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,

The point of the "F" flag is to mark exactly standard libm functions
that have "__builtin_" prefixed to them. The details are in the
comments in Builtins.def, particularly:

//  F -> this is a libc/libm function with a '__builtin_' prefix added.

I still feel like this patch is just cleaning up some omissions in the
C99 floating-point functions.

> that's why Android shouldn't be fiddling with them in the first place.

I agree that we're in __land, but GCC does happily accept this
use-case, and I think there is a precedent for making compatibility
changes like this.

-- Charlie.