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 _______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
