On 05/21/2018 07:19 PM, Jason Merrill wrote:
> On Mon, May 21, 2018 at 9:33 AM, Martin Liška <mli...@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 <mli...@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 <mli...@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 <mli...@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 <ja...@redhat.com>
>>>>>>>>>> Martin Liska <mli...@suse.cz>
>>>>>>>>>> Segher Boessenkool <seg...@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
>
