Re: Builtin expansion versus headers optimization: Reductions

[Mikhail Maltsev <maltsevm at gmail dot com>]

05.06.2015 1:34, Andi Kleen writes:
> OndÅej BÃlka <neleai@seznam.cz> writes:
> 
>> As I commented on libc-alpha list that for string functions a header
>> expansion is better than builtins from maintainance perspective and also
>> that a header is lot easier to write and review than doing that in gcc
>> Jeff said that it belongs to gcc. When I asked about benefits he
>> couldn't say any so I ask again where are benefits of that?
> 
> It definitely has disadvantages in headers. Just today I was prevented
> from setting a conditional break point with strcmp in gdb because
> it expanded to a macro including __extension__ which gdb doesn't
> understand.
> 
There are other issues with macros in glibc headers (well, not as
significant as performance-related concerns, but never the less).

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=24016#c3

> #include <string.h>
> int foo(void *x) {
>     return strcmp(x + 1, "test");
> }
> 
> does not cause warnings when compiled with -Wpointer-arith -O1
> (glibc v. 2.17). It can be reduced to:
> 
> int foo(void *x) {
>     return __extension__({ __builtin_strcmp(x + 1, "test"); });
> }

In this case __extension__ inhibits -Wpointer-arith, because void*
arithmetics is a valid GNU extension, but the user does not even know
that he is using it.
I came across this issue while working with some legacy code and was
rather surprised, that cppcheck (it did not perform the expansion) was
able to find this problem, but GCC was not.

Original case in the bug (Missing warning for unspecified evaluation order):

> Take the following example:
> void f(int *a)
> {
>   *a++ = __extension__ ({ int bb = *a; bb; });
> }
> ---
> We don't warn for the operation on a.
[snip]
> If I remove the declation of bb, it works,
[snip]
> This was reduced from the following code with glibc and -O1:
> #include <ctype.h>
> void strtolower(char *data) { while (*data != '\0') *data++ = tolower(*data); }

Though in this case it seems to be a problem with GCC rather than
headers, absence of macros and block expressions would allow to give a
warning already in current version.

Another issue: strncmp is a macro and it requires exactly 3 arguments.
Arguments are expanded after substitution, So this code does not work
(with -O1+):

/* string literal with length */
#define STRSZ(x) x, (sizeof(x) - 1)
...
bool starts_with_test(const char *s)
{
    return strncmp(s, STRSZ("test")) == 0;
}

One has to write something like this:
#define strncmp_const(s1, s2c) strncmp(s1, s2c, sizeof(s2c) - 1)

> The compiler has much more information than the headers.
> - It can do alias analysis, so to avoid needing to handle overlap
> and similar.
> - It can (sometimes) determine alignment, which is important
> information for tuning.
> - With profile feedback it can use value histograms to determine the
> best code.
Value range information is also passed to, for example, memcpy expander.
Even if exact length of data being copied is not known at compile time,
the known range can help the compiler to select a subset of available
algorithms a choose one of them at run time without using several
branches and a huge jumptable (which is unavoidable in library code,
because it has to deal with general case).

BUT comparison functions (like str[n]cmp, memcmp) expand to something
like "repz cmpsb", which is definitely suboptimal. IMHO, a quick and
dirty (but working) solution would be to use library call instead of
relying on HAVE_cmpstr[n]si, (except short strings). A better solution
would be to implement something like movmem pattern (which is aware of
value ranges, alignment, etc.) but for comparison patterns. I could try
to write at least some prototype for this (if no one else has already
started working on it).

-- 
Regards,
    Mikhail Maltsev