This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]