From: Martin Sebor <msebor@gmail.com>
To: Trevor Saunders <tbsaunde@tbsaunde.org>
Cc: tbsaunde+gcc@tbsaunde.org, gcc-patches@gcc.gnu.org
Subject: Re: [PATCH 00/19] cleanup of memory stats prototypes
Date: Tue, 01 Aug 2017 00:11:00 -0000 [thread overview]
Message-ID: <9894e2eb-cd37-4930-80c3-4acd38f722ef@gmail.com> (raw)
In-Reply-To: <20170731231126.ma3hfycyurf7s67r@ball>
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?
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
next prev parent reply other threads:[~2017-08-01 0:11 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-07-27 8:30 tbsaunde+gcc
2017-07-27 8:30 ` [PATCH 04/19] use c++ for make_int_cst_stat tbsaunde+gcc
2017-07-27 8:30 ` [PATCH 07/19] replace gimple_alloc_stat with c++ tbsaunde+gcc
2017-07-27 8:30 ` [PATCH 10/19] use c++ for tree_cons_stat tbsaunde+gcc
2017-07-27 8:30 ` [PATCH 06/19] use c++ instead of {make,grow}_tree_vec_stat tbsaunde+gcc
2017-07-27 8:30 ` [PATCH 14/19] replace rtx_alloc_stat with c++ tbsaunde+gcc
2017-07-27 8:30 ` [PATCH 08/19] use c++ instead of build_decl_stat tbsaunde+gcc
2017-07-27 8:30 ` [PATCH 03/19] use cxx instead of make_tree_binfo_stat tbsaunde+gcc
2017-07-27 8:31 ` [PATCH 17/19] use c++ for bitmap_initialize tbsaunde+gcc
2017-07-27 8:31 ` [PATCH 12/19] use C++ for {make,build}_vector_stat tbsaunde+gcc
2017-07-27 8:31 ` [PATCH 01/19] use c++ instead of make_node_stat tbsaunde+gcc
2017-07-27 8:31 ` [PATCH 16/19] simplify the bitmap alloc_stat functions with c++ tbsaunde+gcc
2017-07-27 8:31 ` [PATCH 02/19] use c++ instead of _stat for copy_node_stat tbsaunde+gcc
2017-07-27 8:31 ` [PATCH 13/19] use c++ for build_tree_list{,_vec}_stat tbsaunde+gcc
2017-07-27 8:31 ` [PATCH 05/19] use c++ instead of buildN_stat{,_loc} tbsaunde+gcc
2017-07-27 8:31 ` [PATCH 15/19] replace shallow_copy_rtx_stat with c++ tbsaunde+gcc
2017-07-27 8:31 ` [PATCH 11/19] remove unused build_var_debug_value prototype tbsaunde+gcc
2017-07-27 8:31 ` [PATCH 09/19] use c++ instead of build_vl_exp_stat tbsaunde+gcc
2017-07-27 8:31 ` [PATCH 19/19] use c++ for fold_buildN_loc tbsaunde+gcc
2017-07-27 8:31 ` [PATCH 18/19] use c++ for gimple_build_debug_bind{,_source} tbsaunde+gcc
2017-07-27 8:43 ` [PATCH 00/19] cleanup of memory stats prototypes Richard Biener
2017-07-28 16:32 ` Bernhard Reutner-Fischer
2017-07-29 1:08 ` Trevor Saunders
2017-07-29 1:10 ` Trevor Saunders
2017-07-31 18:33 ` Jeff Law
2017-07-31 20:56 ` Martin Sebor
2017-07-31 21:34 ` Trevor Saunders
2017-07-31 22:58 ` Martin Sebor
2017-07-31 23:11 ` Trevor Saunders
2017-08-01 0:11 ` Martin Sebor [this message]
2017-08-01 9:05 ` Richard Biener
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9894e2eb-cd37-4930-80c3-4acd38f722ef@gmail.com \
--to=msebor@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=tbsaunde+gcc@tbsaunde.org \
--cc=tbsaunde@tbsaunde.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).