[patch] don't bzero->memset if cfun is memset

[Mark D. Baushke]

Mark Mitchell writes:
>
>>> And, the patch only goes half way anyhow; what about index and
>>> strchr and other such pairs?
>>
>> I'm willing to patch those also.  I didn't spot them in my initial
>> searches (I did look for other cases, but those are handled
>> differently than bzero), but I see them now.
>
>And there may be even more subtle cases where you go through an
>intermediate function before recurring again.

This is an interesting idea, but I was under the impression that bzero()
is presently a special case as the only builtin which expands to calling
another function rather than calling itself or expanding as inline code.

Do you have any particular examples of functions that are or could be
candidates for an expand_builtin_<foo> treatment with this property?

>>> Otherwise, simply use -fno-builtins.
>>
>> That effects an entire file, not just the one expansion, so isn't
>> always appropriate.
>
>That's why I suggested adding an attribute to the function that's
>being allegedly miscompiled.

What sort of mechanism did you envision? 

  * An attribute that says that the compiler should behave as if
    -fno-builtin had been specified for the entire file when
    compiling an implementation of the function?

       Questions: 
       + How would this interact with static inline functions?
         (ie, is a static inline function template optimized to
          include builtin expansions or is it implicitly disabled
	  due to the attribute on its parent function?)

       + How about nested functions? (Do they inherit the attribute?)

  * An attribute that says that a call to the function should not
    be considered as a candidate for -fbuiltin expansion?
    So, 'void bzero(void *, size_t) __attribute__((nobuiltin));'
    would mean that the compiler should never implicitly do builtin
    expansions for bzero() calls where that attribute is visible.

Then there would also be the question of an __attribute__((builtin))
possibly being available when compiling a file with -fno-builtin on the
command line.

>But my guess is that you could put memset in its own file pretty
>easily.

I do not think that is a good assumption. 

If the intention is to do something particularly 'clever' based on
knowledge of the properties of, for example, address alignment for all
functions that are using memset() in a given file. In that case, you may
really want to have a static inline memset() function in the compilation
unit and moving it out to a separate file would potentially be worse.

>I understand what happenned: you had to debug a program that got an
>infinite recursion and so you changed the compiler.

Actually, the program in question was found in a 'kernel' initialization
driver that was called very shortly after control was transfered from
the boot loader. The compiler was being used as a cross compiler and the
particular embedded environment did not provide a memset() function.

There is no documentation with the compiler that bzero() calls are
morphed into memset() calls that I could find.

The particular kernel had a 'bzero' implementation, but no 'memset'
function. A driver adapted from third-party code had calls to 'memset'
so the engineer coded a quick-n-dirty 'memset' as a static inline
function in a header included by the file so as to avoid dragging in any
more POSIX functions or modifying the original code more than necessary.
(Yes, this is not a wonderful thing to do, he should have followed the
coding standards for that project and converted all of the memset()
calls to bzero, if possible.)

A warning generated by the compiler would have been sufficient to stop
the pain when the new compiler was introduced. However, it was not a
visible change to the compiler behavior and -fbuiltin was useful in many
other places in the kernel code, so it was left as the default.

>It's just that I'm not convinced that it is a bug, and even if I were
>convinced I'm not convinced that your patch is robust and documented.

A more robust solution would be nice and I see that DJ has provided a
newer patch that seems to be more robust to me.

>Maybe you should just warn?

Why is that any better? Mapping bzero() into memset() seems a risky
optimization to me.

I seem to have missed where POSIX, or C99, or perhaps some other
standards organization? have provided the world with official
recognition of the original Berkeley UNIX API. 

My current understanding is that bzero() is a de facto function to
initialize memory that is available in many *NIX-like environments. If it
has been given status as a standard API, then why is it mapping to
memset() instead of an inline copy of itself?

Maybe we need a -fno-berkeley-builtins switch? 1/2 :-)

Mark Mitchell writes in another message:
>We also know that in practice this alleged misfeature is only going to
>occur for people writing C libraries, which is, thankfully, a small
>percentage of all people using the compiler.

Actually, this may also happen to people working with some embedded
systems or with systems that are not yet providing all of the POSIX API
libraries. Yes, it is still a samll percentage of all people using the
compiler, but does that make it easier to ignore questionable
optimizations?

--
Mark Baushke