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

[Martin Liška <mliska at suse dot cz>]

On 05/21/2018 07:19 PM, Jason Merrill wrote:
> 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.

Hello.

I see, not that clang++ also does not emit debug info for a constexprs:

(gdb) list
1	constexpr const char *myconstexpr = "function name";
2	
3	int main()
4	{
5	  __builtin_printf (__PRETTY_FUNCTION__);
6	  __builtin_printf (myconstexpr);
7	}
(gdb) p myconstexpr
$1 = <optimized out>


> 
> 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.

That's promising.

> 
> Why do __*FUNCTION__ cause problems that
> 
> constexpr const char *p = "function name";
> 
> doesn't?

Difference is that for 'p' we do @object in assembly:

	.section	.rodata
.LC0:
	.string	"function name"
	.align 8
	.type	_ZL11myconstexpr, @object
	.size	_ZL11myconstexpr, 8
_ZL11myconstexpr:
	.quad	.LC0

Motivation for the original patch was to remove need of doing a symbol.
The issues I still see is how (and where) to assign to a string constant (of a __PRETTY_FUNCTION name)
a section?

Martin

> 
> Jason
>