diff mbox

[linemap] Make some asserts fail gracefully

Message ID CAESRpQCYZxo4j4Dso_v_3YUNoj3o7BdVAUJLsGBTOvthxLSmgQ@mail.gmail.com
State New
Headers show

Commit Message

Manuel López-Ibáñez Dec. 1, 2014, 4:39 p.m. UTC
The following patch adds linemap_assert_fails(), which is an assert
meant to be used in conditions such as:

if (linemap_assert_fails(EXPR))
  handle_error();

This is useful for ICEs that we would like to detect during the
development phase but that could be handled gracefully. In the case of
linemap_position_for_loc_and_offset (its only user so far), there are
a few conditions that show something went wrong, but it would be bad
to ICE on users when we can simply return the original location
without column offset, which is only a minor diagnostic output
degradation.

Bootstrapped & regression tested.

I'm happy to use a different name. I tried linemap_soft_assert, but it
doesn't read as nice together with "if". The alternative is to just
not use assert but a simple 'if'. We could resort to that if some
condition triggers too often or we cannot find an immediate fix. For
now, one has to use special Fortran testcases to trigger this, thus I
would like to fix the testcases rather than disable the asserts
completely.

OK?

libcpp/ChangeLog:

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

    * include/line-map.h (linemap_assert_fails): Declare.
    * line-map.c (linemap_position_for_loc_and_offset): Use it.

Comments

Dodji Seketeli Dec. 2, 2014, 1:31 p.m. UTC | #1
Manuel López-Ibáñez <lopezibanez@gmail.com> writes:


> +/* Assert that becomes a conditional expression when not checking.

For the sake of clarity towards newcomers, I'd say:

    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 than ICE. Usage:

Similarly, I'd say:

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:

OK with those changes and thank for your time and dedication.

Cheers,
diff mbox

Patch

Index: libcpp/line-map.c
===================================================================
--- libcpp/line-map.c	(revision 218192)
+++ libcpp/line-map.c	(working copy)
@@ -643,11 +643,13 @@  linemap_position_for_loc_and_offset (str
 				     unsigned int offset)
 {
   const struct line_map * map = NULL;
 
   /* This function does not support virtual locations yet.  */
-  linemap_assert (!linemap_location_from_macro_expansion_p (set, loc));
+  if (linemap_assert_fails
+      (!linemap_location_from_macro_expansion_p (set, loc)))
+    return loc;
 
   if (offset == 0
       /* Adding an offset to a reserved location (like
 	 UNKNOWN_LOCATION for the C/C++ FEs) does not really make
 	 sense.  So let's leave the location intact in that case.  */
@@ -656,26 +658,31 @@  linemap_position_for_loc_and_offset (str
 
   /* We find the real location and shift it.  */
   loc = linemap_resolve_location (set, loc, LRK_SPELLING_LOCATION, &map);
   /* The new location (loc + offset) should be higher than the first
      location encoded by MAP.  */
-  linemap_assert (MAP_START_LOCATION (map) < loc + offset);
+  if (linemap_assert_fails (MAP_START_LOCATION (map) < loc + offset))
+    return loc;
 
   /* If MAP is not the last line map of its set, then the new location
      (loc + offset) should be less than the first location encoded by
      the next line map of the set.  */
   if (map != LINEMAPS_LAST_ORDINARY_MAP (set))
-    linemap_assert (loc + offset < MAP_START_LOCATION (&map[1]));
+    if (linemap_assert_fails (loc + offset < MAP_START_LOCATION (&map[1])))
+      return loc;
 
   offset += SOURCE_COLUMN (map, loc);
-  linemap_assert (offset < (1u << map->d.ordinary.column_bits));
+  if (linemap_assert_fails (offset < (1u << map->d.ordinary.column_bits)))
+    return loc;
 
   source_location r = 
     linemap_position_for_line_and_column (map,
 					  SOURCE_LINE (map, loc),
 					  offset);
-  linemap_assert (map == linemap_lookup (set, r));
+  if (linemap_assert_fails (map == linemap_lookup (set, r)))
+    return loc;
+
   return r;
 }
 
 /* Given a virtual source location yielded by a map (either an
    ordinary or a macro map), returns that map.  */
Index: libcpp/include/line-map.h
===================================================================
--- libcpp/include/line-map.h	(revision 218192)
+++ libcpp/include/line-map.h	(working copy)
@@ -578,18 +578,27 @@  bool linemap_location_from_macro_expansi
   do {						\
     if (! (EXPR))				\
       abort ();					\
   } while (0)
 
+/* Assert that becomes a conditional expression when not checking.
+   Use this for conditions that should not happen but if they happen, it
+   is better to handle them gracefully than ICE. Usage:
+
+   if (linemap_assert_fails(EXPR)) handle_error(); */
+#define linemap_assert_fails(EXPR) __extension__ \
+  ({linemap_assert (EXPR); false;}) 
+
 /* 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
 
 /* Encode and return a source_location from a column number. The
    source line considered is the last source line used to call