[OE-core] [PATCH] glibc: add -fno-builtin-strlen when not using -O2

[Andre McCurdy]

On Wed, Dec 14, 2016 at 5:04 PM, Randy MacLeod
<randy.macleod at windriver.com> wrote:
> On 2016-12-13 04:16 AM, Andre McCurdy wrote:
>>
>> On Mon, Dec 12, 2016 at 9:14 PM, Huang, Jie (Jackie)
>> <Jackie.Huang at windriver.com> wrote:
>>>>
>>>> From: Andre McCurdy [mailto:armccurdy at gmail.com]
>>>> For reference, here's the patch I've been using. It's a slightly more
>>>> generic fix than the one in the KDE bug report.
>>>
>>> Thanks, It's a better patch and I will take it and send as v2 of this
>>> issue if you're
>>> not going to send it yourself, is it fine for you and could you provide
>>> extra info
>>> for the patch header like, upstream-status, written by or Signed-off-by?
>>
>> Sure. I forget why I didn't submit this at the time. The full patch is:
>>
>>> From d34e2a50ca5493f5a0ce9ccad83a36ac33689266 Mon Sep 17 00:00:00 2001
>>
>> From: Andre McCurdy <armccurdy at gmail.com>
>> Date: Fri, 12 Feb 2016 18:22:12 -0800
>> Subject: [PATCH] make ld-XXX.so strlen intercept optional
>>
>> Hack: Depending on how glibc was compiled (e.g. optimised for size or
>> built with _FORTIFY_SOURCE enabled) the strlen symbol might not be
>> found in ld-XXX.so. Therefore although we should still try to
>> intercept it, don't make it mandatory to do so.
>>
>> Upstream-Status: Inappropriate
>
> Can you explain why it's not appropriate for upstream?

Because it doesn't take into account the main reason why valgrind
tries to find a strlen symbol in ld-XXX.so, ie as a sanity check that
debug symbols for ld-XXX.so are available. In OE core, it's OK to
relax the sanity check because we ensure that symbols are available
via an RRECOMMENDS in the valgrind recipe:

  # valgrind needs debug information for ld.so at runtime in order to
  # redirect functions like strlen.
  RRECOMMENDS_${PN} += "${TCLIBC}-dbg"

However applying my patch (or the KDE patch referenced by Khem - which
is an even bigger hack) in a build environment which doesn't try to
make the same guarantees might be dangerous.

For reference, Buildroot handles the problem by never fully stripping ld-XXX.so

  https://github.com/buildroot/buildroot/commit/1edb4c51dee78d7d26700c037f29558e73d27ee0

> Does valgrind not support running with different optimizations
> of glibc?

Apparently not, but you should probably ask that question on the
valgrind lists rather than here.

A solution which may be more suitable for upstream would be for
valgrind to check for a number of different symbols (rather than just
strlen) and only generate a fatal error if none can be found. That's
not a trivial patch though.

>> Signed-off-by: Andre McCurdy <armccurdy at gmail.com>
>> ---
>>  coregrind/m_redir.c | 13 ++++++++++++-
>>  1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/coregrind/m_redir.c b/coregrind/m_redir.c
>> index 7e4df8d..640a346 100644
>> --- a/coregrind/m_redir.c
>> +++ b/coregrind/m_redir.c
>> @@ -1220,7 +1220,18 @@ static void add_hardwired_spec (const  HChar*
>> sopatt, const HChar* fnpatt,
>>     spec->from_fnpatt = CONST_CAST(HChar *,fnpatt);
>>     spec->to_addr     = to_addr;
>>     spec->isWrap      = False;
>> -   spec->mandatory   = mandatory;
>> +
>> +   /* Hack: Depending on how glibc was compiled (e.g. optimised for size
>> or
>> +      built with _FORTIFY_SOURCE enabled) the strlen symbol might not be
>> found.
>> +      Therefore although we should still try to intercept it, don't make
>> it
>> +      mandatory to do so. We over-ride "mandatory" here to avoid the need
>> to
>> +      patch the many different architecture specific callers to
>> +      add_hardwired_spec(). */
>> +   if (0==VG_(strcmp)("strlen", fnpatt))
>> +      spec->mandatory = NULL;
>
>
> I know that Jackie has a v2 out but since the interested parties are
> CCed here, and since this is marked as a hack, would there
> be any value in issuing a warning:
>
> #warning "strlen() will not be tracked due to glibc optimization level"
>
> or something like that. Maybe it's overkill since strlen is (should be?)
> side-effect free but I thought I'd share the thought.
>
> What's the right thing to do upstream after we have this hack merged
> locally to fix our ptests?
>
> ../Randy
>
>> +   else
>> +      spec->mandatory = mandatory;
>> +
>>     /* VARIABLE PARTS */
>>     spec->mark        = False; /* not significant */
>>     spec->done        = False; /* not significant */
>>
>
>
> --
> # Randy MacLeod. SMTS, Linux, Wind River
> Direct: 613.963.1350 | 350 Terry Fox Drive, Suite 200, Ottawa, ON, Canada,
> K2K 2W5