diff mbox

[2/4] libcpp: Replace macro usage with C++ constructs

Message ID 1430528215-19648-3-git-send-email-dmalcolm@redhat.com
State New
Headers show

Commit Message

David Malcolm May 2, 2015, 12:56 a.m. UTC
libcpp makes extensive use of the C preprocessor.  Whilst this has a
pleasingly self-referential quality, I find the code hard-to-read;
implementing source location support in my JIT branch was much harder than
I felt it should have been.

In an attempt at making the code easier to follow, and to build towards
a followup patch, this patch converts most of these macros to C++
equivalents: using "const" for compile-time constants, and inline
functions where macros aren't used as lvalues.

This effectively documents the expected types of the params, and makes
them available from the debugger e.g.:

  (gdb) p LINEMAP_FILE ($3)
  $1 = 0x13b8b37 "<command-line>"

and indeed the constants also:

  (gdb) p IS_ADHOC_LOC(MAX_SOURCE_LOCATION)
  $2 = false
  (gdb) p IS_ADHOC_LOC(MAX_SOURCE_LOCATION + 1)
  $3 = true

[I didn't mark the inline functions as "static"; should they be?]

[FWIW, I posted a reduced version of this patch about a year ago as:
  https://gcc.gnu.org/ml/gcc-patches/2014-05/msg01092.html
which covered a smaller subset of the macros].

libcpp/ChangeLog:
	* include/line-map.h (MAX_SOURCE_LOCATION): Convert from a macro
	to a const source_location.
	(RESERVED_LOCATION_COUNT): Likewise.
	(linemap_check_ordinary): Convert from a macro to a pair of inline
	functions, for const/non-const arguments.
	(MAP_START_LOCATION): Likewise.
	(ORDINARY_MAP_STARTING_LINE_NUMBER): Likewise.
	(ORDINARY_MAP_INCLUDER_FILE_INDEX): Likewise.
	(ORDINARY_MAP_IN_SYSTEM_HEADER_P): Likewise.
	(ORDINARY_MAP_NUMBER_OF_COLUMN_BITS): Convert from a macro to a
	pair of inline functions, for const/non-const arguments, where the
	latter is named...
	(SET_ORDINARY_MAP_NUMBER_OF_COLUMN_BITS): New function.
	(ORDINARY_MAP_FILE_NAME): Convert from a macro to a pair of inline
	functions, for const/non-const arguments.
	(MACRO_MAP_MACRO): Likewise.
	(MACRO_MAP_NUM_MACRO_TOKENS): Likewise.
	(MACRO_MAP_LOCATIONS): Likewise.
	(MACRO_MAP_EXPANSION_POINT_LOCATION): Likewise.
	(LINEMAPS_MAP_INFO): Likewise.
	(LINEMAPS_MAPS): Likewise.
	(LINEMAPS_ALLOCATED): Likewise.
	(LINEMAPS_USED): Likewise.
	(LINEMAPS_CACHE): Likewise.
	(LINEMAPS_ORDINARY_CACHE): Likewise.
	(LINEMAPS_MACRO_CACHE): Likewise.
	(LINEMAPS_MAP_AT): Convert from a macro to an inline function.
	(LINEMAPS_LAST_MAP): Likewise.
	(LINEMAPS_LAST_ALLOCATED_MAP): Likewise.
	(LINEMAPS_ORDINARY_MAPS): Likewise.
	(LINEMAPS_ORDINARY_MAP_AT): Likewise.
	(LINEMAPS_ORDINARY_ALLOCATED): Likewise.
	(LINEMAPS_ORDINARY_USED): Likewise.
	(LINEMAPS_LAST_ORDINARY_MAP): Likewise.
	(LINEMAPS_LAST_ALLOCATED_ORDINARY_MAP): Likewise.
	(LINEMAPS_MACRO_MAPS): Likewise.
	(LINEMAPS_MACRO_MAP_AT): Likewise.
	(LINEMAPS_MACRO_ALLOCATED): Likewise.
	(LINEMAPS_MACRO_USED): Likewise.
	(LINEMAPS_MACRO_LOWEST_LOCATION): Likewise.
	(LINEMAPS_LAST_MACRO_MAP): Likewise.
	(LINEMAPS_LAST_ALLOCATED_MACRO_MAP): Likewise.
	(IS_ADHOC_LOC): Likewise.
	(COMBINE_LOCATION_DATA): Likewise.
	(SOURCE_LINE): Likewise.
	(SOURCE_COLUMN): Likewise.
	(LAST_SOURCE_LINE_LOCATION): Likewise.
	(LAST_SOURCE_LINE): Likewise.
	(LAST_SOURCE_COLUMN): Likewise.
	(LAST_SOURCE_LINE_LOCATION)
	(INCLUDED_FROM): Likewise.
	(MAIN_FILE_P): Likewise.
	(LINEMAP_FILE): Likewise.
	(LINEMAP_LINE): Likewise.
	(LINEMAP_SYSP): Likewise.
	(linemap_location_before_p): Likewise.
	* line-map.c (linemap_check_files_exited): Make local "map" const.
	(linemap_add): Use SET_ORDINARY_MAP_NUMBER_OF_COLUMN_BITS.
	(linemap_line_start): Likewise.
---
 libcpp/include/line-map.h | 533 ++++++++++++++++++++++++++++++++++------------
 libcpp/line-map.c         |   6 +-
 2 files changed, 397 insertions(+), 142 deletions(-)

Comments

Jeff Law May 4, 2015, 7:15 p.m. UTC | #1
On 05/01/2015 06:56 PM, David Malcolm wrote:
> libcpp makes extensive use of the C preprocessor.  Whilst this has a
> pleasingly self-referential quality, I find the code hard-to-read;
> implementing source location support in my JIT branch was much harder than
> I felt it should have been.
>
> In an attempt at making the code easier to follow, and to build towards
> a followup patch, this patch converts most of these macros to C++
> equivalents: using "const" for compile-time constants, and inline
> functions where macros aren't used as lvalues.
>
> This effectively documents the expected types of the params, and makes
> them available from the debugger e.g.:
>
>    (gdb) p LINEMAP_FILE ($3)
>    $1 = 0x13b8b37 "<command-line>"
>
> and indeed the constants also:
>
>    (gdb) p IS_ADHOC_LOC(MAX_SOURCE_LOCATION)
>    $2 = false
>    (gdb) p IS_ADHOC_LOC(MAX_SOURCE_LOCATION + 1)
>    $3 = true
>
> [I didn't mark the inline functions as "static"; should they be?]
>
> [FWIW, I posted a reduced version of this patch about a year ago as:
>    https://gcc.gnu.org/ml/gcc-patches/2014-05/msg01092.html
> which covered a smaller subset of the macros].
>
> libcpp/ChangeLog:
> 	* include/line-map.h (MAX_SOURCE_LOCATION): Convert from a macro
> 	to a const source_location.
> 	(RESERVED_LOCATION_COUNT): Likewise.
> 	(linemap_check_ordinary): Convert from a macro to a pair of inline
> 	functions, for const/non-const arguments.
> 	(MAP_START_LOCATION): Likewise.
> 	(ORDINARY_MAP_STARTING_LINE_NUMBER): Likewise.
> 	(ORDINARY_MAP_INCLUDER_FILE_INDEX): Likewise.
> 	(ORDINARY_MAP_IN_SYSTEM_HEADER_P): Likewise.
> 	(ORDINARY_MAP_NUMBER_OF_COLUMN_BITS): Convert from a macro to a
> 	pair of inline functions, for const/non-const arguments, where the
> 	latter is named...
> 	(SET_ORDINARY_MAP_NUMBER_OF_COLUMN_BITS): New function.
> 	(ORDINARY_MAP_FILE_NAME): Convert from a macro to a pair of inline
> 	functions, for const/non-const arguments.
> 	(MACRO_MAP_MACRO): Likewise.
> 	(MACRO_MAP_NUM_MACRO_TOKENS): Likewise.
> 	(MACRO_MAP_LOCATIONS): Likewise.
> 	(MACRO_MAP_EXPANSION_POINT_LOCATION): Likewise.
> 	(LINEMAPS_MAP_INFO): Likewise.
> 	(LINEMAPS_MAPS): Likewise.
> 	(LINEMAPS_ALLOCATED): Likewise.
> 	(LINEMAPS_USED): Likewise.
> 	(LINEMAPS_CACHE): Likewise.
> 	(LINEMAPS_ORDINARY_CACHE): Likewise.
> 	(LINEMAPS_MACRO_CACHE): Likewise.
> 	(LINEMAPS_MAP_AT): Convert from a macro to an inline function.
> 	(LINEMAPS_LAST_MAP): Likewise.
> 	(LINEMAPS_LAST_ALLOCATED_MAP): Likewise.
> 	(LINEMAPS_ORDINARY_MAPS): Likewise.
> 	(LINEMAPS_ORDINARY_MAP_AT): Likewise.
> 	(LINEMAPS_ORDINARY_ALLOCATED): Likewise.
> 	(LINEMAPS_ORDINARY_USED): Likewise.
> 	(LINEMAPS_LAST_ORDINARY_MAP): Likewise.
> 	(LINEMAPS_LAST_ALLOCATED_ORDINARY_MAP): Likewise.
> 	(LINEMAPS_MACRO_MAPS): Likewise.
> 	(LINEMAPS_MACRO_MAP_AT): Likewise.
> 	(LINEMAPS_MACRO_ALLOCATED): Likewise.
> 	(LINEMAPS_MACRO_USED): Likewise.
> 	(LINEMAPS_MACRO_LOWEST_LOCATION): Likewise.
> 	(LINEMAPS_LAST_MACRO_MAP): Likewise.
> 	(LINEMAPS_LAST_ALLOCATED_MACRO_MAP): Likewise.
> 	(IS_ADHOC_LOC): Likewise.
> 	(COMBINE_LOCATION_DATA): Likewise.
> 	(SOURCE_LINE): Likewise.
> 	(SOURCE_COLUMN): Likewise.
> 	(LAST_SOURCE_LINE_LOCATION): Likewise.
> 	(LAST_SOURCE_LINE): Likewise.
> 	(LAST_SOURCE_COLUMN): Likewise.
> 	(LAST_SOURCE_LINE_LOCATION)
> 	(INCLUDED_FROM): Likewise.
> 	(MAIN_FILE_P): Likewise.
> 	(LINEMAP_FILE): Likewise.
> 	(LINEMAP_LINE): Likewise.
> 	(LINEMAP_SYSP): Likewise.
> 	(linemap_location_before_p): Likewise.
> 	* line-map.c (linemap_check_files_exited): Make local "map" const.
> 	(linemap_add): Use SET_ORDINARY_MAP_NUMBER_OF_COLUMN_BITS.
> 	(linemap_line_start): Likewise.
> ---
> -#define MAP_START_LOCATION(MAP) (MAP)->start_location
> +#if defined ENABLE_CHECKING && (GCC_VERSION >= 2007)
> +
> +/* Assertion macro to be used in line-map code.  */
> +#define linemap_assert(EXPR)                  \
> +  do {                                                \
> +    if (! (EXPR))                             \
> +      abort ();                                       \
> +  } while (0)
> +
> +/* Assert that becomes a conditional expression when checking is disabled at
> +   compilation time.  Use this for conditions that should not happen but if
> +   they happen, it is better to handle them gracefully rather than crash
> +   randomly later.
> +   Usage:
> +
> +   if (linemap_assert_fails(EXPR)) handle_error(); */
> +#define linemap_assert_fails(EXPR) __extension__ \
> +  ({linemap_assert (EXPR); false;})
> +
> +#else
> +/* Include EXPR, so that unused variable warnings do not occur.  */
> +#define linemap_assert(EXPR) ((void)(0 && (EXPR)))
> +#define linemap_assert_fails(EXPR) (! (EXPR))
> +#endif
So if we're generally trying to get away from #define programming, then 
this part seems like a bit of a step backwards.

The definition of linemap_assert in the #else clause seems odd.  Aren't 
you worried that the compiler will short-circuit the test?    If EXPR 
has side effects, then that's clearly not good.

I note you've got an abort () in here, that's generally frowned upon in 
libraries.


>
> -#define ORDINARY_MAP_FILE_NAME(MAP) \
> -  linemap_check_ordinary (MAP)->d.ordinary.to_file
> +/* Return TRUE if MAP encodes locations coming from a macro
> +   replacement-list at macro expansion point.  */
> +bool linemap_macro_expansion_map_p (const struct line_map *);
> +
> +/* Assert that MAP encodes locations of tokens that are not part of
> +   the replacement-list of a macro expansion.  */
> +inline struct line_map *linemap_check_ordinary (struct line_map *map)
Return type and inline declarations on a different line than the 
function name/arguments.

> -#define LINEMAPS_MAPS(SET, MAP_KIND) \
> -  (LINEMAPS_MAP_INFO (SET, MAP_KIND))->maps
> +inline line_map *
> +LINEMAPS_MAPS (const line_maps *set, bool map_kind)
> +{
> +  return LINEMAPS_MAP_INFO (set, map_kind)->maps;
> +}
> +
> +inline line_map * &
> +LINEMAPS_MAPS (line_maps *set, bool map_kind)
> +{
> +  return LINEMAPS_MAP_INFO (set, map_kind)->maps;
> +}
Do we really need two implementation of those various functions, one for 
const line_maps * the other for line_maps *?

While I suspect ICF ultimately should merge these functions, I'd really 
prefer to avoid the duplication.

Note that many of the macros didn't have comments (they should have, but 
it's often missed), correcting that as you turn them into functions 
would be greatly appreciated.

Generally, I like what I see.  There's some minor questions/issues to 
address, so let's go with another iteration.

jeff
David Malcolm May 5, 2015, 6:21 p.m. UTC | #2
On Mon, 2015-05-04 at 13:15 -0600, Jeff Law wrote:
On 05/01/2015 06:56 PM, David Malcolm wrote:
> > libcpp makes extensive use of the C preprocessor.  Whilst this has a
> > pleasingly self-referential quality, I find the code hard-to-read;
> > implementing source location support in my JIT branch was much harder than
> > I felt it should have been.
> >
> > In an attempt at making the code easier to follow, and to build towards
> > a followup patch, this patch converts most of these macros to C++
> > equivalents: using "const" for compile-time constants, and inline
> > functions where macros aren't used as lvalues.
> >
> > This effectively documents the expected types of the params, and makes
> > them available from the debugger e.g.:
> >
> >    (gdb) p LINEMAP_FILE ($3)
> >    $1 = 0x13b8b37 "<command-line>"
> >
> > and indeed the constants also:
> >
> >    (gdb) p IS_ADHOC_LOC(MAX_SOURCE_LOCATION)
> >    $2 = false
> >    (gdb) p IS_ADHOC_LOC(MAX_SOURCE_LOCATION + 1)
> >    $3 = true
> >
> > [I didn't mark the inline functions as "static"; should they be?]
> >
> > [FWIW, I posted a reduced version of this patch about a year ago as:
> >    https://gcc.gnu.org/ml/gcc-patches/2014-05/msg01092.html
> > which covered a smaller subset of the macros].
> >
> > libcpp/ChangeLog:
> > 	* include/line-map.h (MAX_SOURCE_LOCATION): Convert from a macro
> > 	to a const source_location.
> > 	(RESERVED_LOCATION_COUNT): Likewise.
> > 	(linemap_check_ordinary): Convert from a macro to a pair of inline
> > 	functions, for const/non-const arguments.
> > 	(MAP_START_LOCATION): Likewise.
> > 	(ORDINARY_MAP_STARTING_LINE_NUMBER): Likewise.
> > 	(ORDINARY_MAP_INCLUDER_FILE_INDEX): Likewise.
> > 	(ORDINARY_MAP_IN_SYSTEM_HEADER_P): Likewise.
> > 	(ORDINARY_MAP_NUMBER_OF_COLUMN_BITS): Convert from a macro to a
> > 	pair of inline functions, for const/non-const arguments, where the
> > 	latter is named...
> > 	(SET_ORDINARY_MAP_NUMBER_OF_COLUMN_BITS): New function.
> > 	(ORDINARY_MAP_FILE_NAME): Convert from a macro to a pair of inline
> > 	functions, for const/non-const arguments.
> > 	(MACRO_MAP_MACRO): Likewise.
> > 	(MACRO_MAP_NUM_MACRO_TOKENS): Likewise.
> > 	(MACRO_MAP_LOCATIONS): Likewise.
> > 	(MACRO_MAP_EXPANSION_POINT_LOCATION): Likewise.
> > 	(LINEMAPS_MAP_INFO): Likewise.
> > 	(LINEMAPS_MAPS): Likewise.
> > 	(LINEMAPS_ALLOCATED): Likewise.
> > 	(LINEMAPS_USED): Likewise.
> > 	(LINEMAPS_CACHE): Likewise.
> > 	(LINEMAPS_ORDINARY_CACHE): Likewise.
> > 	(LINEMAPS_MACRO_CACHE): Likewise.
> > 	(LINEMAPS_MAP_AT): Convert from a macro to an inline function.
> > 	(LINEMAPS_LAST_MAP): Likewise.
> > 	(LINEMAPS_LAST_ALLOCATED_MAP): Likewise.
> > 	(LINEMAPS_ORDINARY_MAPS): Likewise.
> > 	(LINEMAPS_ORDINARY_MAP_AT): Likewise.
> > 	(LINEMAPS_ORDINARY_ALLOCATED): Likewise.
> > 	(LINEMAPS_ORDINARY_USED): Likewise.
> > 	(LINEMAPS_LAST_ORDINARY_MAP): Likewise.
> > 	(LINEMAPS_LAST_ALLOCATED_ORDINARY_MAP): Likewise.
> > 	(LINEMAPS_MACRO_MAPS): Likewise.
> > 	(LINEMAPS_MACRO_MAP_AT): Likewise.
> > 	(LINEMAPS_MACRO_ALLOCATED): Likewise.
> > 	(LINEMAPS_MACRO_USED): Likewise.
> > 	(LINEMAPS_MACRO_LOWEST_LOCATION): Likewise.
> > 	(LINEMAPS_LAST_MACRO_MAP): Likewise.
> > 	(LINEMAPS_LAST_ALLOCATED_MACRO_MAP): Likewise.
> > 	(IS_ADHOC_LOC): Likewise.
> > 	(COMBINE_LOCATION_DATA): Likewise.
> > 	(SOURCE_LINE): Likewise.
> > 	(SOURCE_COLUMN): Likewise.
> > 	(LAST_SOURCE_LINE_LOCATION): Likewise.
> > 	(LAST_SOURCE_LINE): Likewise.
> > 	(LAST_SOURCE_COLUMN): Likewise.
> > 	(LAST_SOURCE_LINE_LOCATION)
> > 	(INCLUDED_FROM): Likewise.
> > 	(MAIN_FILE_P): Likewise.
> > 	(LINEMAP_FILE): Likewise.
> > 	(LINEMAP_LINE): Likewise.
> > 	(LINEMAP_SYSP): Likewise.
> > 	(linemap_location_before_p): Likewise.
> > 	* line-map.c (linemap_check_files_exited): Make local "map" const.
> > 	(linemap_add): Use SET_ORDINARY_MAP_NUMBER_OF_COLUMN_BITS.
> > 	(linemap_line_start): Likewise.
> > ---
> > -#define MAP_START_LOCATION(MAP) (MAP)->start_location
> > +#if defined ENABLE_CHECKING && (GCC_VERSION >= 2007)
> > +
> > +/* Assertion macro to be used in line-map code.  */
> > +#define linemap_assert(EXPR)                  \
> > +  do {                                                \
> > +    if (! (EXPR))                             \
> > +      abort ();                                       \
> > +  } while (0)
> > +
> > +/* Assert that becomes a conditional expression when checking is disabled at
> > +   compilation time.  Use this for conditions that should not happen but if
> > +   they happen, it is better to handle them gracefully rather than crash
> > +   randomly later.
> > +   Usage:
> > +
> > +   if (linemap_assert_fails(EXPR)) handle_error(); */
> > +#define linemap_assert_fails(EXPR) __extension__ \
> > +  ({linemap_assert (EXPR); false;})
> > +
> > +#else
> > +/* Include EXPR, so that unused variable warnings do not occur.  */
> > +#define linemap_assert(EXPR) ((void)(0 && (EXPR)))
> > +#define linemap_assert_fails(EXPR) (! (EXPR))
> > +#endif
> 
I moved linemap_assert and linemap_assert_fails higher up within the
file so that they can appear before the bodies of the inline functions.
I didn't change their implementation; it's a simple cut-and-paste, so
these hunks are just an artifact of git's diff-ing algorithm getting
confused by that.

Should I have called that out in the ChangeLog entries?

I've split patch 2 of the original kit into two parts, one containing
the move of these macros (and the decl of linemap_macro_expansion_map_p)
to higher up within the file, then a followup patch containing the
"real" changes.  (assuming they're OK, should I commit them separately,
for ease of seeing this in our SVN history?)

> So if we're generally trying to get away from #define programming, then 
> this part seems like a bit of a step backwards.
> 
> The definition of linemap_assert in the #else clause seems odd.  Aren't 
> you worried that the compiler will short-circuit the test?    If EXPR 
> has side effects, then that's clearly not good.
> 
The (0 && ....) part of this was added in r217473
(aka 36a4e8f0351cdac4befb259ad9ef1738498cdf4e):

2014-11-13  Manuel López-Ibáñez  <manu@gcc.gnu.org>

       * include/line-map.h: Include EXPR, so that unused variable warnings
       do not occur.

which was due to:
https://gcc.gnu.org/ml/gcc-patches/2014-11/msg01240.html

> I note you've got an abort () in here, that's generally frowned upon in 
> libraries.
> 
Agreed, though it's an assertion; it's not clear to me what else we
could do.  As noted above, I'm not changing this code, simply moving it
higher up in the header.

> -#define ORDINARY_MAP_FILE_NAME(MAP) \
> > -  linemap_check_ordinary (MAP)->d.ordinary.to_file
> > +/* Return TRUE if MAP encodes locations coming from a macro
> > +   replacement-list at macro expansion point.  */
> > +bool linemap_macro_expansion_map_p (const struct line_map *);
> > +
> > +/* Assert that MAP encodes locations of tokens that are not part of
> > +   the replacement-list of a macro expansion.  */
> > +inline struct line_map *linemap_check_ordinary (struct line_map *map)
> Return type and inline declarations on a different line than the 
> function name/arguments.
> 
(nods; will fix)

> -#define LINEMAPS_MAPS(SET, MAP_KIND) \
> > -  (LINEMAPS_MAP_INFO (SET, MAP_KIND))->maps
> > +inline line_map *
> > +LINEMAPS_MAPS (const line_maps *set, bool map_kind)
> > +{
> > +  return LINEMAPS_MAP_INFO (set, map_kind)->maps;
> > +}
> > +
> > +inline line_map * &
> > +LINEMAPS_MAPS (line_maps *set, bool map_kind)
> > +{
> > +  return LINEMAPS_MAP_INFO (set, map_kind)->maps;
> > +}
> Do we really need two implementation of those various functions, one for 
> const line_maps * the other for line_maps *?
> 

The issue here is when they are accessed as lvalues, rather than just as
rvalues.

I've had a go at eliminating some of the lvalue ones as a followup
patch: given that after patch 4 these become just field accesses, it's
trivial to turn the usage sites into plain accesses of fields.

Is that desirable, or are the uses of the macros regarded as useful
(e.g. for grepping)?  (my preference is for the former: to turn them
into plain field accesses at those sites)

Ironically, one of the ones that isn't so easy to eliminate is the
one you called out above.  Eliminating those ones made the code messier,
so I kept those ones.

> While I suspect ICF ultimately should merge these functions, I'd really 
> prefer to avoid the duplication.
> 
> Note that many of the macros didn't have comments (they should have, but 
> it's often missed), correcting that as you turn them into functions 
> would be greatly appreciated.
> 
> Generally, I like what I see.  There's some minor questions/issues to 
> address, so let's go with another iteration.
> 
> jeff

Thanks.

I'm sending updated patches; the old patch 2 split into 2 parts, and
a followup ("patch 5 of 4", as it were):

  [PATCH 2/4 v2: part 1] Move linemap_assert higher up within the file
  [PATCH 2/4 v2: part 2] libcpp: Replace macro usage with C++ constructs
  [PATCH 5/4] libcpp: Eliminate most of the non-const/reference-returning inline fns

Do I need to do any performance testing of this kit?  Given that the
resulting inline functions become trivial field lookups in patch 4,
I'm assuming that our inliner is good enough to optimize it as well
as the status quo implementation.

Also, for comments before functions, do we prefer a blank line between
the comment and decl, or for them to abutt?

/* Foo.  */

foo ();

vs

/* Bar.  */
bar ();

How do the following look?

Thanks
Dave

 gcc/ada/gcc-interface/trans.c |   2 +-
 gcc/c-family/c-common.h       |   4 +-
 gcc/c-family/c-lex.c          |   6 +-
 gcc/c-family/c-opts.c         |  14 +-
 gcc/c-family/c-ppoutput.c     |   6 +-
 gcc/common.opt                |   4 +
 gcc/diagnostic.c              |   2 +-
 gcc/fortran/cpp.c             |  12 +-
 gcc/genmatch.c                |   4 +-
 gcc/input.c                   | 244 ++++++++++++++-
 gcc/input.h                   |   2 +
 gcc/java/jcf-parse.c          |   2 +-
 gcc/toplev.c                  |   3 +
 gcc/tree-diagnostic.c         |   9 +-
 libcpp/directives.c           |  17 +-
 libcpp/files.c                |   2 +-
 libcpp/include/cpplib.h       |   2 +-
 libcpp/include/line-map.h     | 667 ++++++++++++++++++++++++++++++------------
 libcpp/internal.h             |  12 +-
 libcpp/line-map.c             | 266 +++++++++--------
 libcpp/location-example.txt   | 216 ++++++++++++++
 libcpp/macro.c                |  18 +-
 22 files changed, 1150 insertions(+), 364 deletions(-)
 create mode 100644 libcpp/location-example.txt
Jeff Law May 8, 2015, 9:45 p.m. UTC | #3
On 05/05/2015 12:21 PM, David Malcolm wrote:
> On Mon, 2015-05-04 at 13:15 -0600, Jeff Law wrote:
> On 05/01/2015 06:56 PM, David Malcolm wrote:
>>>
>>> [I didn't mark the inline functions as "static"; should they be?]
Just a follow-up on this.  I got burned by the ODR issues around 
implicit extern inlines earlier this week.  Basically I had two distinct 
implementations for an inline function with the same name.  They 
obviously appeared in different "header files".

When optimizng, this was fine as they'd actually get inlined and all was 
good.  Things blew up when the optimizer was off.  We got two functions 
with the same name, but different implementations.  The linker blindly 
chose one for me, and in one context it was the wrong one.

This led to bootstrap comparison failures.

So, just be careful :-)

>>
> I moved linemap_assert and linemap_assert_fails higher up within the
> file so that they can appear before the bodies of the inline functions.
> I didn't change their implementation; it's a simple cut-and-paste, so
> these hunks are just an artifact of git's diff-ing algorithm getting
> confused by that.
>
> Should I have called that out in the ChangeLog entries?
It might have helped.  Or you could have pulled it out like you did in 
the follow-up.  No worries.  So consider the issues raised in that code 
as a non-issues for your patch.  They'd be nice things to clean up 
someday though.

>> -#define LINEMAPS_MAPS(SET, MAP_KIND) \
>>> -  (LINEMAPS_MAP_INFO (SET, MAP_KIND))->maps
>>> +inline line_map *
>>> +LINEMAPS_MAPS (const line_maps *set, bool map_kind)
>>> +{
>>> +  return LINEMAPS_MAP_INFO (set, map_kind)->maps;
>>> +}
>>> +
>>> +inline line_map * &
>>> +LINEMAPS_MAPS (line_maps *set, bool map_kind)
>>> +{
>>> +  return LINEMAPS_MAP_INFO (set, map_kind)->maps;
>>> +}
>> Do we really need two implementation of those various functions, one for
>> const line_maps * the other for line_maps *?
>>
>
> The issue here is when they are accessed as lvalues, rather than just as
> rvalues.
>
> I've had a go at eliminating some of the lvalue ones as a followup
> patch: given that after patch 4 these become just field accesses, it's
> trivial to turn the usage sites into plain accesses of fields.
>
> Is that desirable, or are the uses of the macros regarded as useful
> (e.g. for grepping)?  (my preference is for the former: to turn them
> into plain field accesses at those sites)
I think macros would only be useful for those already familiar with the 
code.  If I'm looking for how a particular field gets accessed, I'm 
generally going to be grepping for the field.

Of course, if the field have names that are common ("next" anyone?), 
then, well, grepping for field names is pointless.


>
> Ironically, one of the ones that isn't so easy to eliminate is the
> one you called out above.  Eliminating those ones made the code messier,
> so I kept those ones.
lol.  I just picked one after seeing the pattern repeat a bit.
>
> I'm sending updated patches; the old patch 2 split into 2 parts, and
> a followup ("patch 5 of 4", as it were):
>
>    [PATCH 2/4 v2: part 1] Move linemap_assert higher up within the file
>    [PATCH 2/4 v2: part 2] libcpp: Replace macro usage with C++ constructs
>    [PATCH 5/4] libcpp: Eliminate most of the non-const/reference-returning inline fns
>
> Do I need to do any performance testing of this kit?  Given that the
> resulting inline functions become trivial field lookups in patch 4,
> I'm assuming that our inliner is good enough to optimize it as well
> as the status quo implementation.
Yea, I wouldn't worry about performance of an inline field access vs a 
macro.  We should be good to go.

>
> Also, for comments before functions, do we prefer a blank line between
> the comment and decl, or for them to abutt?
>
> /* Foo.  */
>
> foo ();
>
> vs
>
> /* Bar.  */
> bar ();
>
> How do the following look?
I've done both in my days. I try to follow whatever is in the nearby code.

Let me have a look at the followup...

jeff
Jan Hubicka May 10, 2015, 3:39 p.m. UTC | #4
> On 05/05/2015 12:21 PM, David Malcolm wrote:
> >On Mon, 2015-05-04 at 13:15 -0600, Jeff Law wrote:
> >On 05/01/2015 06:56 PM, David Malcolm wrote:
> >>>
> >>>[I didn't mark the inline functions as "static"; should they be?]
> Just a follow-up on this.  I got burned by the ODR issues around
> implicit extern inlines earlier this week.  Basically I had two
> distinct implementations for an inline function with the same name.
> They obviously appeared in different "header files".
> 
> When optimizng, this was fine as they'd actually get inlined and all
> was good.  Things blew up when the optimizer was off.  We got two
> functions with the same name, but different implementations.  The
> linker blindly chose one for me, and in one context it was the wrong
> one.
> 
> This led to bootstrap comparison failures.
> 
> So, just be careful :-)

Martin actually tried using ICF to warn about such cases at LTO time (if the
two COMDAT function bodies does not seem to have same implementation, complain
about it). One showstopper here is that early inliner may divere easily (if you
provide inline implementation of called function in one unit), but perhaps we could
output warnings at least in cases we know that both optimization flags and
inline decisions match. (this can be checked during the compare by looking
at DECL_INITIAL block structure)

In GCC I would really love us to switch to non-static inline and just chase
out ODR violations if they exists (hopefully it is not that common). This
makes code reuse more explicit to IPA and hopefully will lead to smaller
binaries.

Honza
diff mbox

Patch

diff --git a/libcpp/include/line-map.h b/libcpp/include/line-map.h
index e1681ea..021ca6e 100644
--- a/libcpp/include/line-map.h
+++ b/libcpp/include/line-map.h
@@ -155,7 +155,7 @@  struct GTY(()) line_map_ordinary {
 
 /* This is the highest possible source location encoded within an
    ordinary or macro map.  */
-#define MAX_SOURCE_LOCATION 0x7FFFFFFF
+const source_location MAX_SOURCE_LOCATION = 0x7FFFFFFF;
 
 struct cpp_hashnode;
 
@@ -279,30 +279,168 @@  struct GTY(()) line_map {
   } GTY((desc ("%1.reason == LC_ENTER_MACRO"))) d;
 };
 
-#define MAP_START_LOCATION(MAP) (MAP)->start_location
+#if defined ENABLE_CHECKING && (GCC_VERSION >= 2007)
+
+/* Assertion macro to be used in line-map code.  */
+#define linemap_assert(EXPR)                  \
+  do {                                                \
+    if (! (EXPR))                             \
+      abort ();                                       \
+  } while (0)
+
+/* Assert that becomes a conditional expression when checking is disabled at
+   compilation time.  Use this for conditions that should not happen but if
+   they happen, it is better to handle them gracefully rather than crash
+   randomly later.
+   Usage:
+
+   if (linemap_assert_fails(EXPR)) handle_error(); */
+#define linemap_assert_fails(EXPR) __extension__ \
+  ({linemap_assert (EXPR); false;})
+
+#else
+/* Include EXPR, so that unused variable warnings do not occur.  */
+#define linemap_assert(EXPR) ((void)(0 && (EXPR)))
+#define linemap_assert_fails(EXPR) (! (EXPR))
+#endif
 
-#define ORDINARY_MAP_FILE_NAME(MAP) \
-  linemap_check_ordinary (MAP)->d.ordinary.to_file
+/* Return TRUE if MAP encodes locations coming from a macro
+   replacement-list at macro expansion point.  */
+bool linemap_macro_expansion_map_p (const struct line_map *);
+
+/* Assert that MAP encodes locations of tokens that are not part of
+   the replacement-list of a macro expansion.  */
+inline struct line_map *linemap_check_ordinary (struct line_map *map)
+{
+  linemap_assert (!linemap_macro_expansion_map_p (map));
+  return map;
+}
+
+inline const struct line_map *
+linemap_check_ordinary (const struct line_map *map)
+{
+  linemap_assert (!linemap_macro_expansion_map_p (map));
+  return map;
+}
 
-#define ORDINARY_MAP_STARTING_LINE_NUMBER(MAP) \
-  linemap_check_ordinary (MAP)->d.ordinary.to_line
+inline source_location
+MAP_START_LOCATION (const line_map *map)
+{
+  return map->start_location;
+}
 
-#define ORDINARY_MAP_INCLUDER_FILE_INDEX(MAP) \
-  linemap_check_ordinary (MAP)->d.ordinary.included_from
+inline source_location&
+MAP_START_LOCATION (line_map *map)
+{
+  return map->start_location;
+}
 
-#define ORDINARY_MAP_IN_SYSTEM_HEADER_P(MAP) \
-  linemap_check_ordinary (MAP)->d.ordinary.sysp
+inline linenum_type
+ORDINARY_MAP_STARTING_LINE_NUMBER (const line_map *map)
+{
+  return linemap_check_ordinary (map)->d.ordinary.to_line;
+}
 
-#define ORDINARY_MAP_NUMBER_OF_COLUMN_BITS(MAP) \
-  linemap_check_ordinary (MAP)->d.ordinary.column_bits
+inline linenum_type&
+ORDINARY_MAP_STARTING_LINE_NUMBER (line_map *map)
+{
+  return linemap_check_ordinary (map)->d.ordinary.to_line;
+}
 
-#define MACRO_MAP_MACRO(MAP) (MAP)->d.macro.macro
+inline int
+ORDINARY_MAP_INCLUDER_FILE_INDEX (const line_map *map)
+{
+  return linemap_check_ordinary (map)->d.ordinary.included_from;
+}
 
-#define MACRO_MAP_NUM_MACRO_TOKENS(MAP) (MAP)->d.macro.n_tokens
+inline int&
+ORDINARY_MAP_INCLUDER_FILE_INDEX (line_map *map)
+{
+  return linemap_check_ordinary (map)->d.ordinary.included_from;
+}
 
-#define MACRO_MAP_LOCATIONS(MAP) (MAP)->d.macro.macro_locations
+inline unsigned char
+ORDINARY_MAP_IN_SYSTEM_HEADER_P (const line_map *map)
+{
+  return linemap_check_ordinary (map)->d.ordinary.sysp;
+}
 
-#define MACRO_MAP_EXPANSION_POINT_LOCATION(MAP) (MAP)->d.macro.expansion
+inline unsigned char &
+ORDINARY_MAP_IN_SYSTEM_HEADER_P (line_map *map)
+{
+  return linemap_check_ordinary (map)->d.ordinary.sysp;
+}
+
+inline unsigned char
+ORDINARY_MAP_NUMBER_OF_COLUMN_BITS (const line_map *map)
+{
+  return linemap_check_ordinary (map)->d.ordinary.column_bits;
+}
+inline void
+SET_ORDINARY_MAP_NUMBER_OF_COLUMN_BITS (line_map *map, int col_bits)
+{
+  linemap_check_ordinary (map)->d.ordinary.column_bits = col_bits;
+}
+
+inline const char *
+ORDINARY_MAP_FILE_NAME (const line_map *map)
+{
+  return linemap_check_ordinary (map)->d.ordinary.to_file;
+}
+
+inline const char * &
+ORDINARY_MAP_FILE_NAME (line_map *map)
+{
+  return linemap_check_ordinary (map)->d.ordinary.to_file;
+}
+
+inline cpp_hashnode *
+MACRO_MAP_MACRO (const line_map *map)
+{
+  return map->d.macro.macro;
+}
+
+inline cpp_hashnode * &
+MACRO_MAP_MACRO (line_map *map)
+{
+  return map->d.macro.macro;
+}
+
+inline unsigned int
+MACRO_MAP_NUM_MACRO_TOKENS (const line_map *map)
+{
+  return map->d.macro.n_tokens;
+}
+
+inline unsigned int &
+MACRO_MAP_NUM_MACRO_TOKENS (line_map *map)
+{
+  return map->d.macro.n_tokens;
+}
+
+inline source_location *
+MACRO_MAP_LOCATIONS (const line_map *map)
+{
+  return map->d.macro.macro_locations;
+}
+
+inline source_location * &
+MACRO_MAP_LOCATIONS (line_map *map)
+{
+  return map->d.macro.macro_locations;
+}
+
+inline source_location
+MACRO_MAP_EXPANSION_POINT_LOCATION (const line_map *map)
+{
+  return map->d.macro.expansion;
+}
+
+inline source_location &
+MACRO_MAP_EXPANSION_POINT_LOCATION (line_map *map)
+{
+  return map->d.macro.expansion;
+}
 
 /* The abstraction of a set of location maps. There can be several
    types of location maps. This abstraction contains the attributes
@@ -391,119 +529,233 @@  struct GTY(()) line_maps {
 /* Returns the pointer to the memory region where information about
    maps are stored in the line table SET. MACRO_MAP_P is a flag
    telling if we want macro or ordinary maps.  */
-#define LINEMAPS_MAP_INFO(SET, MACRO_MAP_P)				\
-  ((MACRO_MAP_P)							\
-   ? &((SET)->info_macro)						\
-   : &((SET)->info_ordinary))
+inline struct maps_info *
+LINEMAPS_MAP_INFO (line_maps *set, bool macro_map_p)
+{
+  return (macro_map_p
+	  ? &(set->info_macro)
+	  : &(set->info_ordinary));
+}
+
+inline const struct maps_info *
+LINEMAPS_MAP_INFO (const line_maps *set, bool macro_map_p)
+{
+  return (macro_map_p
+	  ? &(set->info_macro)
+	  : &(set->info_ordinary));
+}
 
 /* Returns the pointer to the memory region where maps are stored in
    the line table SET. MAP_KIND shall be TRUE if we are interested in
    macro maps false otherwise.  */
-#define LINEMAPS_MAPS(SET, MAP_KIND) \
-  (LINEMAPS_MAP_INFO (SET, MAP_KIND))->maps
+inline line_map *
+LINEMAPS_MAPS (const line_maps *set, bool map_kind)
+{
+  return LINEMAPS_MAP_INFO (set, map_kind)->maps;
+}
+
+inline line_map * &
+LINEMAPS_MAPS (line_maps *set, bool map_kind)
+{
+  return LINEMAPS_MAP_INFO (set, map_kind)->maps;
+}
 
 /* Returns the number of allocated maps so far. MAP_KIND shall be TRUE
    if we are interested in macro maps, FALSE otherwise.  */
-#define LINEMAPS_ALLOCATED(SET, MAP_KIND) \
-  (LINEMAPS_MAP_INFO (SET, MAP_KIND))->allocated
+inline unsigned int
+LINEMAPS_ALLOCATED (const line_maps *set, bool map_kind)
+{
+  return LINEMAPS_MAP_INFO (set, map_kind)->allocated;
+}
+
+inline unsigned int &
+LINEMAPS_ALLOCATED (line_maps *set, bool map_kind)
+{
+  return LINEMAPS_MAP_INFO (set, map_kind)->allocated;
+}
 
 /* Returns the number of used maps so far. MAP_KIND shall be TRUE if
    we are interested in macro maps, FALSE otherwise.*/
-#define LINEMAPS_USED(SET, MAP_KIND) \
-  (LINEMAPS_MAP_INFO (SET, MAP_KIND))->used
+inline unsigned int
+LINEMAPS_USED (const line_maps *set, bool map_kind)
+{
+  return LINEMAPS_MAP_INFO (set, map_kind)->used;
+}
+
+inline unsigned int &
+LINEMAPS_USED (line_maps *set, bool map_kind)
+{
+  return LINEMAPS_MAP_INFO (set, map_kind)->used;
+}
 
 /* Returns the index of the last map that was looked up with
    linemap_lookup. MAP_KIND shall be TRUE if we are interested in
    macro maps, FALSE otherwise.  */
-#define LINEMAPS_CACHE(SET, MAP_KIND) \
-  (LINEMAPS_MAP_INFO (SET, MAP_KIND))->cache
+inline unsigned int
+LINEMAPS_CACHE (const line_maps *set, bool map_kind)
+{
+  return LINEMAPS_MAP_INFO (set, map_kind)->cache;
+}
+
+inline unsigned int &
+LINEMAPS_CACHE (line_maps *set, bool map_kind)
+{
+  return LINEMAPS_MAP_INFO (set, map_kind)->cache;
+}
 
 /* Return the map at a given index.  */
-#define LINEMAPS_MAP_AT(SET, MAP_KIND, INDEX)	\
-  (&((LINEMAPS_MAPS (SET, MAP_KIND))[(INDEX)]))
+inline line_map *
+LINEMAPS_MAP_AT (const line_maps *set, bool map_kind, int index)
+{
+  return &(LINEMAPS_MAPS (set, map_kind)[index]);
+}
 
 /* Returns the last map used in the line table SET. MAP_KIND
    shall be TRUE if we are interested in macro maps, FALSE
    otherwise.*/
-#define LINEMAPS_LAST_MAP(SET, MAP_KIND) \
-  LINEMAPS_MAP_AT (SET, MAP_KIND, (LINEMAPS_USED (SET, MAP_KIND) - 1))
+inline line_map *
+LINEMAPS_LAST_MAP (const line_maps *set, bool map_kind)
+{
+  return LINEMAPS_MAP_AT (set, map_kind,
+			  LINEMAPS_USED (set, map_kind) - 1);
+}
 
 /* Returns the last map that was allocated in the line table SET.
    MAP_KIND shall be TRUE if we are interested in macro maps, FALSE
    otherwise.*/
-#define LINEMAPS_LAST_ALLOCATED_MAP(SET, MAP_KIND) \
-  LINEMAPS_MAP_AT (SET, MAP_KIND, LINEMAPS_ALLOCATED (SET, MAP_KIND) - 1)
+inline line_map *
+LINEMAPS_LAST_ALLOCATED_MAP (const line_maps *set, bool map_kind)
+{
+  return LINEMAPS_MAP_AT (set, map_kind,
+			  LINEMAPS_ALLOCATED (set, map_kind) - 1);
+}
 
 /* Returns a pointer to the memory region where ordinary maps are
    allocated in the line table SET.  */
-#define LINEMAPS_ORDINARY_MAPS(SET) \
-  LINEMAPS_MAPS (SET, false)
+inline line_map *
+LINEMAPS_ORDINARY_MAPS (const line_maps *set)
+{
+  return LINEMAPS_MAPS (set, false);
+}
 
 /* Returns the INDEXth ordinary map.  */
-#define LINEMAPS_ORDINARY_MAP_AT(SET, INDEX)	\
-  LINEMAPS_MAP_AT (SET, false, INDEX)
+inline line_map *
+LINEMAPS_ORDINARY_MAP_AT (const line_maps *set, int index)
+{
+  return LINEMAPS_MAP_AT (set, false, index);
+}
 
 /* Return the number of ordinary maps allocated in the line table
    SET.  */
-#define LINEMAPS_ORDINARY_ALLOCATED(SET) \
-  LINEMAPS_ALLOCATED(SET, false)
+inline unsigned int
+LINEMAPS_ORDINARY_ALLOCATED (const line_maps *set)
+{
+  return LINEMAPS_ALLOCATED (set, false);
+}
 
 /* Return the number of ordinary maps used in the line table SET.  */
-#define LINEMAPS_ORDINARY_USED(SET) \
-  LINEMAPS_USED(SET, false)
+inline unsigned int
+LINEMAPS_ORDINARY_USED (const line_maps *set)
+{
+  return LINEMAPS_USED (set, false);
+}
 
 /* Return the index of the last ordinary map that was looked up with
    linemap_lookup.  */
-#define LINEMAPS_ORDINARY_CACHE(SET) \
-  LINEMAPS_CACHE(SET, false)
+inline unsigned int
+LINEMAPS_ORDINARY_CACHE (const line_maps *set)
+{
+  return LINEMAPS_CACHE (set, false);
+}
+
+inline unsigned int &
+LINEMAPS_ORDINARY_CACHE (line_maps *set)
+{
+  return LINEMAPS_CACHE (set, false);
+}
 
 /* Returns a pointer to the last ordinary map used in the line table
    SET.  */
-#define LINEMAPS_LAST_ORDINARY_MAP(SET) \
-  LINEMAPS_LAST_MAP(SET, false)
+inline line_map *
+LINEMAPS_LAST_ORDINARY_MAP (const line_maps *set)
+{
+  return LINEMAPS_LAST_MAP (set, false);
+}
 
 /* Returns a pointer to the last ordinary map allocated the line table
    SET.  */
-#define LINEMAPS_LAST_ALLOCATED_ORDINARY_MAP(SET) \
-  LINEMAPS_LAST_ALLOCATED_MAP(SET, false)
+inline line_map *
+LINEMAPS_LAST_ALLOCATED_ORDINARY_MAP (const line_maps *set)
+{
+  return LINEMAPS_LAST_ALLOCATED_MAP (set, false);
+}
 
 /* Returns a pointer to the beginning of the region where macro maps
    are allcoated.  */
-#define LINEMAPS_MACRO_MAPS(SET) \
-  LINEMAPS_MAPS(SET, true)
+inline line_map *
+LINEMAPS_MACRO_MAPS (const line_maps *set)
+{
+  return LINEMAPS_MAPS (set, true);
+}
 
 /* Returns the INDEXth macro map.  */
-#define LINEMAPS_MACRO_MAP_AT(SET, INDEX)	\
-  LINEMAPS_MAP_AT (SET, true, INDEX)
+inline line_map *LINEMAPS_MACRO_MAP_AT (const line_maps *set, int index)
+{
+  return LINEMAPS_MAP_AT (set, true, index);
+}
 
 /* Returns the number of macro maps that were allocated in the line
    table SET.  */
-#define LINEMAPS_MACRO_ALLOCATED(SET) \
-  LINEMAPS_ALLOCATED(SET, true)
+inline unsigned int
+LINEMAPS_MACRO_ALLOCATED (const line_maps *set)
+{
+  return LINEMAPS_ALLOCATED (set, true);
+}
 
 /* Returns the number of macro maps used in the line table SET.  */
-#define LINEMAPS_MACRO_USED(SET) \
-  LINEMAPS_USED(SET, true)
+inline unsigned int
+LINEMAPS_MACRO_USED (const line_maps *set)
+{
+  return LINEMAPS_USED (set, true);
+}
 
 /* Returns the index of the last macro map looked up with
    linemap_lookup.  */
-#define LINEMAPS_MACRO_CACHE(SET) \
-  LINEMAPS_CACHE(SET, true)
+inline unsigned int
+LINEMAPS_MACRO_CACHE (const line_maps *set)
+{
+  return LINEMAPS_CACHE (set, true);
+}
 
-/* Returns the lowest location [of a token resulting from macro
-   expansion] encoded in this line table.  */
-#define LINEMAPS_MACRO_LOWEST_LOCATION(SET)			\
-  (LINEMAPS_MACRO_USED (set)					\
-   ? MAP_START_LOCATION (LINEMAPS_LAST_MACRO_MAP (set))		\
-   : MAX_SOURCE_LOCATION)
+inline unsigned int &
+LINEMAPS_MACRO_CACHE (line_maps *set)
+{
+  return LINEMAPS_CACHE (set, true);
+}
 
 /* Returns the last macro map used in the line table SET.  */
-#define LINEMAPS_LAST_MACRO_MAP(SET) \
-  LINEMAPS_LAST_MAP (SET, true)
+inline line_map *
+LINEMAPS_LAST_MACRO_MAP (const line_maps *set)
+{
+  return LINEMAPS_LAST_MAP (set, true);
+}
+
+/* Returns the lowest location [of a token resulting from macro
+   expansion] encoded in this line table.  */
+inline source_location
+LINEMAPS_MACRO_LOWEST_LOCATION (const line_maps *set)
+{
+  return LINEMAPS_MACRO_USED (set)
+         ? MAP_START_LOCATION (LINEMAPS_LAST_MACRO_MAP (set))
+         : MAX_SOURCE_LOCATION;
+}
 
 /* Returns the last macro map allocated in the line table SET.  */
-#define LINEMAPS_LAST_ALLOCATED_MACRO_MAP(SET) \
-  LINEMAPS_LAST_ALLOCATED_MAP (SET, true)
+inline line_map *
+LINEMAPS_LAST_ALLOCATED_MACRO_MAP (const line_maps *set)
+{
+  return LINEMAPS_LAST_ALLOCATED_MAP (set, true);
+}
 
 extern void location_adhoc_data_fini (struct line_maps *);
 extern source_location get_combined_adhoc_loc (struct line_maps *,
@@ -512,9 +764,18 @@  extern void *get_data_from_adhoc_loc (struct line_maps *, source_location);
 extern source_location get_location_from_adhoc_loc (struct line_maps *,
 						    source_location);
 
-#define IS_ADHOC_LOC(LOC) (((LOC) & MAX_SOURCE_LOCATION) != (LOC))
-#define COMBINE_LOCATION_DATA(SET, LOC, BLOCK) \
-  get_combined_adhoc_loc ((SET), (LOC), (BLOCK))
+inline bool IS_ADHOC_LOC (source_location loc)
+{
+  return (loc & MAX_SOURCE_LOCATION) != loc;
+}
+
+inline source_location
+COMBINE_LOCATION_DATA (struct line_maps *set,
+		       source_location loc,
+		       void *block)
+{
+  return get_combined_adhoc_loc (set, loc, block);
+}
 
 extern void rebuild_location_adhoc_htab (struct line_maps *);
 
@@ -568,10 +829,6 @@  extern const struct line_map *linemap_lookup
    macro expansion, FALSE otherwise.  */
 bool linemap_tracks_macro_expansion_locs_p (struct line_maps *);
 
-/* Return TRUE if MAP encodes locations coming from a macro
-   replacement-list at macro expansion point.  */
-bool linemap_macro_expansion_map_p (const struct line_map *);
-
 /* Return the name of the macro associated to MACRO_MAP.  */
 const char* linemap_map_get_macro_name (const struct line_map*);
 
@@ -596,78 +853,66 @@  bool linemap_location_from_macro_expansion_p (const struct line_maps *,
 /* source_location values from 0 to RESERVED_LOCATION_COUNT-1 will
    be reserved for libcpp user as special values, no token from libcpp
    will contain any of those locations.  */
-#define RESERVED_LOCATION_COUNT	2
+const int RESERVED_LOCATION_COUNT = 2;
 
 /* Converts a map and a source_location to source line.  */
-#define SOURCE_LINE(MAP, LOC)						\
-  (((((LOC) - linemap_check_ordinary (MAP)->start_location)		\
-     >> (MAP)->d.ordinary.column_bits) + (MAP)->d.ordinary.to_line))
+inline linenum_type
+SOURCE_LINE (const struct line_map *map, source_location loc)
+{
+  return ((loc - linemap_check_ordinary (map)->start_location)
+	  >> map->d.ordinary.column_bits) + map->d.ordinary.to_line;
+}
 
 /* Convert a map and source_location to source column number.  */
-#define SOURCE_COLUMN(MAP, LOC)						\
-  ((((LOC) - linemap_check_ordinary (MAP)->start_location)		\
-    & ((1 << (MAP)->d.ordinary.column_bits) - 1)))
+inline linenum_type
+SOURCE_COLUMN (const struct line_map *map, source_location loc)
+{
+  return ((loc - linemap_check_ordinary (map)->start_location)
+	  & ((1 << map->d.ordinary.column_bits) - 1));
+}
+
+/* Return the location of the last source line within an ordinary
+   map.  */
+inline source_location
+LAST_SOURCE_LINE_LOCATION (const struct line_map *map)
+{
+  return (((linemap_check_ordinary (map)[1].start_location - 1
+	    - map->start_location)
+	   & ~((1 << map->d.ordinary.column_bits) - 1))
+	  + map->start_location);
+}
 
 /* Returns the last source line number within an ordinary map.  This
    is the (last) line of the #include, or other directive, that caused
    a map change.  */
-#define LAST_SOURCE_LINE(MAP) \
-  SOURCE_LINE (MAP, LAST_SOURCE_LINE_LOCATION (MAP))
+inline linenum_type LAST_SOURCE_LINE (const struct line_map *map)
+{
+  return SOURCE_LINE (map, LAST_SOURCE_LINE_LOCATION (map));
+}
 
 /* Return the last column number within an ordinary map.  */
-#define LAST_SOURCE_COLUMN(MAP) \
-  SOURCE_COLUMN (MAP, LAST_SOURCE_LINE_LOCATION (MAP))
 
-/* Return the location of the last source line within an ordinary
-   map.  */
-#define LAST_SOURCE_LINE_LOCATION(MAP)					\
-  ((((linemap_check_ordinary (MAP)[1].start_location - 1		\
-      - (MAP)->start_location)						\
-     & ~((1 << (MAP)->d.ordinary.column_bits) - 1))			\
-    + (MAP)->start_location))
+inline linenum_type LAST_SOURCE_COLUMN (const struct line_map *map)
+{
+  return SOURCE_COLUMN (map, LAST_SOURCE_LINE_LOCATION (map));
+}
 
 /* Returns the map a given map was included from, or NULL if the map
    belongs to the main file, i.e, a file that wasn't included by
    another one.  */
-#define INCLUDED_FROM(SET, MAP)						\
-  ((linemap_check_ordinary (MAP)->d.ordinary.included_from == -1)	\
-   ? NULL								\
-   : (&LINEMAPS_ORDINARY_MAPS (SET)[(MAP)->d.ordinary.included_from]))
-
-/* Nonzero if the map is at the bottom of the include stack.  */
-#define MAIN_FILE_P(MAP)						\
-  ((linemap_check_ordinary (MAP)->d.ordinary.included_from < 0))
-
-#if defined ENABLE_CHECKING && (GCC_VERSION >= 2007)
-
-/* Assertion macro to be used in line-map code.  */
-#define linemap_assert(EXPR)			\
-  do {						\
-    if (! (EXPR))				\
-      abort ();					\
-  } while (0)
- 
-/* Assert that becomes a conditional expression when checking is disabled at
-   compilation time.  Use this for conditions that should not happen but if
-   they happen, it is better to handle them gracefully rather than crash
-   randomly later. 
-   Usage:
-
-   if (linemap_assert_fails(EXPR)) handle_error(); */
-#define linemap_assert_fails(EXPR) __extension__ \
-  ({linemap_assert (EXPR); false;}) 
+inline struct line_map *
+INCLUDED_FROM (struct line_maps *set, const struct line_map *map)
+{
+  return ((linemap_check_ordinary (map)->d.ordinary.included_from == -1)
+	  ? NULL
+	  : (&LINEMAPS_ORDINARY_MAPS (set)[(map)->d.ordinary.included_from]));
+}
 
-/* Assert that MAP encodes locations of tokens that are not part of
-   the replacement-list of a macro expansion.  */
-#define linemap_check_ordinary(LINE_MAP) __extension__		\
-  ({linemap_assert (!linemap_macro_expansion_map_p (LINE_MAP)); \
-    (LINE_MAP);})
-#else
-/* Include EXPR, so that unused variable warnings do not occur.  */
-#define linemap_assert(EXPR) ((void)(0 && (EXPR)))
-#define linemap_assert_fails(EXPR) (! (EXPR))
-#define linemap_check_ordinary(LINE_MAP) (LINE_MAP)
-#endif
+/* True if the map is at the bottom of the include stack.  */
+inline bool MAIN_FILE_P (const struct line_map *map)
+{
+  return linemap_check_ordinary (map)->d.ordinary.included_from < 0;
+}
 
 /* Encode and return a source_location from a column number. The
    source line considered is the last source line used to call
@@ -691,19 +936,25 @@  linemap_position_for_loc_and_offset (struct line_maps *set,
 				     unsigned int offset);
 
 /* Return the file this map is for.  */
-#define LINEMAP_FILE(MAP)					\
-  (linemap_check_ordinary (MAP)->d.ordinary.to_file)
+inline const char * LINEMAP_FILE (const struct line_map *map)
+{
+  return linemap_check_ordinary (map)->d.ordinary.to_file;
+}
 
 /* Return the line number this map started encoding location from.  */
-#define LINEMAP_LINE(MAP)					\
-  (linemap_check_ordinary (MAP)->d.ordinary.to_line)
+inline linenum_type LINEMAP_LINE (const struct line_map *map)
+{
+  return linemap_check_ordinary (map)->d.ordinary.to_line;
+}
 
 /* Return a positive value if map encodes locations from a system
    header, 0 otherwise. Returns 1 if MAP encodes locations in a
    system header and 2 if it encodes locations in a C system header
    that therefore needs to be extern "C" protected in C++.  */
-#define LINEMAP_SYSP(MAP)					\
-  (linemap_check_ordinary (MAP)->d.ordinary.sysp)
+inline unsigned char LINEMAP_SYSP (const struct line_map *map)
+{
+  return linemap_check_ordinary (map)->d.ordinary.sysp;
+}
 
 /* Return a positive value if PRE denotes the location of a token that
    comes before the token of POST, 0 if PRE denotes the location of
@@ -716,8 +967,12 @@  int linemap_compare_locations (struct line_maps *set,
 /* Return TRUE if LOC_A denotes the location a token that comes
    topogically before the token denoted by location LOC_B, or if they
    are equal.  */
-#define linemap_location_before_p(SET, LOC_A, LOC_B)	\
-  (linemap_compare_locations ((SET), (LOC_A), (LOC_B)) >= 0)
+inline bool linemap_location_before_p (struct line_maps *set,
+				       source_location loc_a,
+				       source_location loc_b)
+{
+  return linemap_compare_locations (set, loc_a, loc_b) >= 0;
+}
 
 typedef struct
 {
diff --git a/libcpp/line-map.c b/libcpp/line-map.c
index 1db59a7..fd16c24 100644
--- a/libcpp/line-map.c
+++ b/libcpp/line-map.c
@@ -191,7 +191,7 @@  linemap_init (struct line_maps *set,
 void
 linemap_check_files_exited (struct line_maps *set)
 {
-  struct line_map *map;
+  const struct line_map *map;
   /* Depending upon whether we are handling preprocessed input or
      not, this can be a user error or an ICE.  */
   for (map = LINEMAPS_LAST_ORDINARY_MAP (set);
@@ -371,7 +371,7 @@  linemap_add (struct line_maps *set, enum lc_reason reason,
   ORDINARY_MAP_FILE_NAME (map) = to_file;
   ORDINARY_MAP_STARTING_LINE_NUMBER (map) = to_line;
   LINEMAPS_ORDINARY_CACHE (set) = LINEMAPS_ORDINARY_USED (set) - 1;
-  ORDINARY_MAP_NUMBER_OF_COLUMN_BITS (map) = 0;
+  SET_ORDINARY_MAP_NUMBER_OF_COLUMN_BITS (map, 0);
   set->highest_location = start_location;
   set->highest_line = start_location;
   set->max_column_hint = 0;
@@ -564,7 +564,7 @@  linemap_line_start (struct line_maps *set, linenum_type to_line,
 					       (map),
 					       ORDINARY_MAP_FILE_NAME (map),
 					       to_line);
-      ORDINARY_MAP_NUMBER_OF_COLUMN_BITS (map) = column_bits;
+      SET_ORDINARY_MAP_NUMBER_OF_COLUMN_BITS (map, column_bits);
       r = (MAP_START_LOCATION (map)
 	   + ((to_line - ORDINARY_MAP_STARTING_LINE_NUMBER (map))
 	      << column_bits));