Re: [PATCH 00/19] cleanup of memory stats prototypes

[Richard Biener <richard dot guenther at gmail dot com>]

On Tue, Aug 1, 2017 at 2:11 AM, Martin Sebor <msebor@gmail.com> wrote:
> On 07/31/2017 05:11 PM, Trevor Saunders wrote:
>>
>> On Mon, Jul 31, 2017 at 04:58:15PM -0600, Martin Sebor wrote:
>>>
>>> On 07/31/2017 03:34 PM, Trevor Saunders wrote:
>>>>
>>>> On Mon, Jul 31, 2017 at 02:56:40PM -0600, Martin Sebor wrote:
>>>>>
>>>>> On 07/27/2017 02:30 AM, tbsaunde+gcc@tbsaunde.org wrote:
>>>>>>
>>>>>> From: Trevor Saunders <tbsaunde+gcc@tbsaunde.org>
>>>>>>
>>>>>> The preC++ way of passing information about the call site of a
>>>>>> function was to
>>>>>> use a macro that passed __file__, __LINE__, and __FUNCTION__ to a
>>>>>> function with
>>>>>> the same name with _stat appended to it.  The way this is now done
>>>>>> with C++ is
>>>>>> to have arguments where the default value is __LINE__, __FILE__, and
>>>>>> __FUNCTION__ in the caller.  This has the significant advantage that
>>>>>> if you
>>>>>> look for "^function (" you find the correct function, where in the C
>>>>>> way of
>>>>>> doing things you need to realize its a macro and check the definition
>>>>>> of the
>>>>>> macro to see what to look for next.  So this removes a layer of
>>>>>> indirection,
>>>>>> and makes things somewhat more consistant in using the C++ way of
>>>>>> doing things.
>>>>>
>>>>>
>>>>> So that's what these things are for! :)
>>>>>
>>>>> I must be missing something either about the macros or about your
>>>>> changes.  The way I read the changes it seems that it's no longer
>>>>> possible to call, say,
>>>>>
>>>>>   t = make_node (code);
>>>>>
>>>>> and have __FILE__, __LINE__, and __FUNCTION__ passed as arguments
>>>>> behind the scenes.
>>>>>
>>>>> Instead, to achieve that, make_node has to be called like so
>>>>>
>>>>>   t = make_node (code PASS_MEM_STAT);
>>>>>
>>>>> Otherwise calling make_node() with just one argument will end up
>>>>> calling it with the defaults.
>>>>
>>>>
>>>> calling it with the defaults will do the same thing the macro used to
>>>> do, so its fine unless you want to pass in a location that you got as an
>>>> argument.
>>>
>>>
>>> Sorry, I'm still not sure I understand.  Please bear with me
>>> if I'm just being slow.
>>>
>>> When GATHER_STATISTICS is defined, make_node_stat is declared
>>> like so in current GCC (without your patch):
>>>
>>>   extern tree
>>>   make_node_stat (enum tree_code , const char * _loc_name,
>>>                   int _loc_line,  const char * _loc_function);
>>>
>>> and the make_node macro like so:
>>>
>>>   #define make_node(t) make_node_stat (t MEM_STAT_INFO)
>>>
>>> with MEM_STAT_INFO being defined as follows:
>>>
>>>   #define MEM_STAT_INFO , ALONE_MEM_STAT_INFO
>>>   #define ALONE_MEM_STAT_INFO __FILE__, __LINE__, __FUNCTION__
>>>
>>> So a call to
>>>
>>>   t = make_node (code);
>>>
>>> expands to
>>>
>>>   t = make_node_stat (code, __FILE__, __LINE__, __FUNCTION__);
>>>
>>> If I'm reading your patch correctly (please correct me if/where
>>> I'm not) then it treats the same call as just an ordinary call
>>> to the make_node function with no macro expansion taking place
>>> at the call  site.  I.e., it will be made with the _loc_name,
>>> _loc_line, and _loc_function arguments set to whatever their
>>> defaults are where the function is declared.  To have the call
>>> do the same thing as before it will have to be made with explicit
>>> arguments, e.g., like so:
>>>
>>>   t = make_node (code MEM_STAT_INFO);
>>>
>>> That seems like a (significant) difference.
>>>
>>>> I'm guessing you are missing that the functions when used as the default
>>>> argument provide the location of the call site of the function, not the
>>>> location of the prototype.  Which is not the most obvious thing.
>>>
>>>
>>> I am indeed missing that.  How do/can they do that when that's
>>> not how C++ 98 default arguments work?  (It's only possible with
>>> GCC's __builtin_{FILE,LINE,FUNCTION} but not without these
>>> extensions.)
>>
>>
>> They use the extensions if available, and otherwise provide the wrong
>> values.  However that's fine because this is just for profiling gcc
>> itself so demanding you use gcc 4.8 or greater, or bootstrap the
>> compiler you are going to profile isn't a big deal.
>
>
> Ah, okay, thanks for your patience with me.  I didn't realize
> some language features/extensions get enabled as GCC bootstraps.
>
>>
>> perhaps we should just make --enable-gather-detailed-mem-statss require
>> a recent gcc instead of supporting it but providing bad data for some
>> things?

My plan was to remove the fallback (so you'd get no data).  But yes,
a configure test for the extension would work as well of course.

Note it only affects non-bootstrapped builds or stage1 so the configure
test would be a bit tricky?

Of course I never use mem-stats for a bootstrapped compiler.

Richard.

>
> That would make sense to me.  Either that, or documenting this
> caveat as a known limitation somewhere users of the option will
> notice (or both).
>
> Martin