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

[Jason Merrill <jason at redhat dot com>]

On Mon, May 21, 2018 at 9:33 AM, Martin Liška <mliska@suse.cz> wrote:
> On 10/24/2017 10:24 PM, Jason Merrill wrote:
>> 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?
>
> Going through unresolved issues I still have in ML, I noticed this one. As perfectly analyzed here:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70422#c7
>
> The issue is that we can't find proper function decl in dwarf2out.c because it's function declaration
> that was inlined. And such references are only seen in debug mode.
>
> The only solution I see is DECL_IGNORED_P, which was also suggested by Richi in:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70422#c9
>
> Note that both LLVM and ICC don't allow:
> $ 3       __builtin_printf ("%s\n", __PRETTY_FUNCTION__);
> (gdb) p __PRETTY_FUNCTION__
> No symbol "__PRETTY_FUNCTION__" in current context.
>
> Would it be feasible solution to ignore the declaration?

It seems a rather user-unfriendly solution; being able to look at the
value of an identifier used in the program is a pretty basic
expectation for the debugging experience.

In Jim's comment you mention, he says, "It seems like there are some
conflicting goals here. We want to share the string across functions,
but we also want to put it in function specific comdat sections."

The recent discussion thread at
https://gcc.gnu.org/ml/gcc/2018-05/threads.html#00109 deals with a
very similar issue, of wanting to both merge constant data and
associate it with a function.  The discussion there seems to be
tending toward merging rather than function-grouping, which seems to
match the desire in this PR.

Why do __*FUNCTION__ cause problems that

constexpr const char *p = "function name";

doesn't?

Jason