Re: [PATCH] Make __FUNCTION__ a mergeable string and do not generate symbol entry.

[Jason Merrill <jason at redhat dot com>]

On Thu, Sep 14, 2017 at 5:22 AM, Martin Liška <mliska@suse.cz> wrote:
> On 08/10/2017 09:43 PM, Jason Merrill wrote:
>> On 07/14/2017 01:35 AM, Martin Liška wrote:
>>> On 05/01/2017 09:13 PM, Jason Merrill wrote:
>>>> On Wed, Apr 26, 2017 at 6:58 AM, Martin Liška <mliska@suse.cz> wrote:
>>>>> On 04/25/2017 01:58 PM, Jakub Jelinek wrote:
>>>>>> On Tue, Apr 25, 2017 at 01:48:05PM +0200, Martin Liška wrote:
>>>>>>> Hello.
>>>>>>>
>>>>>>> This is patch that was originally installed by Jason and later reverted due to PR70422.
>>>>>>> In the later PR Richi suggested a fix for that and Segher verified that it helped him
>>>>>>> to survive regression tests. That's reason why I'm resending that.
>>>>>>>
>>>>>>> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
>>>>>>>
>>>>>>> Ready to be installed?
>>>>>>> Martin
>>>>>>
>>>>>>> >From a34ce0ef37ae00609c9f3ff98a9cb0b7db6a8bd0 Mon Sep 17 00:00:00 2001
>>>>>>> From: marxin <mliska@suse.cz>
>>>>>>> Date: Thu, 20 Apr 2017 14:56:30 +0200
>>>>>>> Subject: [PATCH] Make __FUNCTION__ a mergeable string and do not generate
>>>>>>>   symbol entry.
>>>>>>>
>>>>>>> gcc/cp/ChangeLog:
>>>>>>>
>>>>>>> 2017-04-20  Jason Merrill  <jason@redhat.com>
>>>>>>>           Martin Liska  <mliska@suse.cz>
>>>>>>>           Segher Boessenkool  <segher@kernel.crashing.org>
>>>>>>>
>>>>>>>       PR c++/64266
>>>>>>>       PR c++/70353
>>>>>>>       PR bootstrap/70422
>>>>>>>       Core issue 1962
>>>>>>>       * decl.c (cp_fname_init): Decay the initializer to pointer.
>>>>>>>       (cp_make_fname_decl): Set DECL_DECLARED_CONSTEXPR_P,
>>>>>>>       * pt.c (tsubst_expr) [DECL_EXPR]: Set DECL_VALUE_EXPR,
>>>>>>>       DECL_INITIALIZED_BY_CONSTANT_EXPRESSION_P and
>>>>>>>       DECL_IGNORED_P.  Don't call cp_finish_decl.
>>>>>>
>>>>>> If we don't emit those into the debug info, will the debugger be
>>>>>> able to handle __FUNCTION__ etc. properly?
>>>>>
>>>>> No, debugger with the patch can't handled these. Similar to how clang
>>>>> behaves currently. Maybe it can be conditionally enabled with -g3, or -g?
>>>>>
>>>>>> Admittedly, right now we emit it into debug info only if those decls
>>>>>> are actually used, say on:
>>>>>> const char * foo () { return __FUNCTION__; }
>>>>>> const char * bar () { return ""; }
>>>>>> we'd emit foo::__FUNCTION__, but not bar::__FUNCTION__, so the debugger
>>>>>> has to have some handling of it anyway.  But while in functions
>>>>>> that don't refer to __FUNCTION__ it is always the debugger that needs
>>>>>> to synthetize those and thus they will be always pointer-equal,
>>>>>> if there are some uses of it and for other uses the debugger would
>>>>>> synthetize it, there is the possibility that the debugger synthetized
>>>>>> string will not be the same object as actually used in the function.
>>>>>
>>>>> You're right, currently one has to use a special function to be able to
>>>>> print it in debugger. I believe we've already discussed that, according
>>>>> to spec, the strings don't have to point to a same string.
>>>>>
>>>>> Suggestions what we should do with the patch?
>>>>
>>>> We need to emit debug information for these variables.  From Jim's
>>>> description in 70422 it seems that the problem is that the reference
>>>> to the string from the debug information is breaking
>>>> function_mergeable_rodata_prefix, which relies on
>>>> current_function_decl.  It seems to me that its callers should pass
>>>> along their decl parameter so that f_m_r_p can use the decl's
>>>> DECL_CONTEXT rather than rely on current_function_decl being set
>>>> properly>
>>>> Jason
>>>>
>>>
>>> Ok, after some time I returned back to it. I followed your advises and
>>> changed the function function_mergeable_rodata_prefix. Apart from a small
>>> rebase was needed.
>>>
>>> May I ask Jim to test the patch?
>>> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
>>
>>> +  DECL_IGNORED_P (decl) = 1;
>>
>> As I said before, we do need to emit debug information for these variables, so this is wrong.
>
> Hello.
>
> Sorry for overlooking that.
>
>>
>>> -  section *s = targetm.asm_out.function_rodata_section (current_function_decl);
>>> +  tree decl = current_function_decl;
>>> +  if (decl && DECL_CONTEXT (decl)
>>> +      && TREE_CODE (DECL_CONTEXT (decl)) == FUNCTION_DECL)
>>> +    decl = DECL_CONTEXT (decl);
>>
>> I don't see how this would help; it still relies on current_function_decl being set correctly, which was the problem we were running into before.
>
> I see, that's what I wanted to discuss on Cauldron with you, but eventually I did not find time.
> Well problem that I see is that we want to make decision based on DECL_CONTEXT(fname_decl), but mergeable_string_section
> is called with STRING_CST (which is VALUE_EXPR of created fname_decl):
[...]
> And idea how to resolve this?

Well, when we're being called from the expander, current_function_decl is fine.

Hmm, why would current_function_decl be wrong in dwarf2out?  Can we fix that?

Why does your change to function_mergeable_rodata_prefix make any difference?

Jason