diff mbox series

C++ PATCH for debug/65821, source location and default arguments

Message ID CADzB+2mZTfNohgtu58v3m=q4TdG1fvPSUqUFuDnq1Js0vj+Fgg@mail.gmail.com
State New
Headers show
Series C++ PATCH for debug/65821, source location and default arguments | expand

Commit Message

Jason Merrill April 10, 2018, 2:21 p.m. UTC 
65821 complains that in this testcase, setting a breakpoint on main()
ends up stopping at the line with the default argument of foo().  I
think I agree that the confusion from this debugging experience
outweighs the usefulness of being able to step through evaluation of a
default argument, so this patch changes all expressions from a default
argument to have the location of the call, rather than only calls to
the source-location built-ins.

Tested x86_64-pc-linux-gnu, applying to trunk.

Comments

Jason Merrill April 10, 2018, 5:18 p.m. UTC | #1 
On Tue, Apr 10, 2018 at 10:21 AM, Jason Merrill <jason@redhat.com> wrote:
> 65821 complains that in this testcase, setting a breakpoint on main()
> ends up stopping at the line with the default argument of foo().  I
> think I agree that the confusion from this debugging experience
> outweighs the usefulness of being able to step through evaluation of a
> default argument, so this patch changes all expressions from a default
> argument to have the location of the call, rather than only calls to
> the source-location built-ins.

Oops, that broke the libstdc++ source_location/1.cc test, which needs
the same treatment for NSDMIs, which also makes sense.  So, different
approach:

Tested x86_64-pc-linux-gnu, applying to trunk.
commit b1a319de39267a858e7bc1ea02ab40fdbbb8f062
Author: Jason Merrill <jason@redhat.com>
Date:   Tue Apr 10 12:45:46 2018 -0400

            PR debug/65821 - wrong location for main().
    
            * call.c (clear_location_r, convert_default_arg): Revert.
            * tree.c (break_out_target_exprs): Add clear_location parm.
            (struct bot_data): New.
            (bot_manip): Clear location if requested.
            * init.c (get_nsdmi): Pass clear_location.

diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index 94226d6ea71..38b16ba991e 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -7296,21 +7296,6 @@ cxx_type_promotes_to (tree type)
   return promote;
 }
 
-/* walk_tree callback to override EXPR_LOCATION in an expression tree.  */
-
-tree
-clear_location_r (tree *tp, int *walk_subtrees, void */*data*/)
-{
-  if (!EXPR_P (*tp))
-    {
-      *walk_subtrees = 0;
-      return NULL_TREE;
-    }
-  if (EXPR_HAS_LOCATION (*tp))
-    SET_EXPR_LOCATION (*tp, input_location);
-  return NULL_TREE;
-}
-
 /* ARG is a default argument expression being passed to a parameter of
    the indicated TYPE, which is a parameter to FN.  PARMNUM is the
    zero-based argument number.  Do any required conversions.  Return
@@ -7374,11 +7359,7 @@ convert_default_arg (tree type, tree arg, tree fn, int parmnum,
   push_deferring_access_checks (dk_no_check);
   /* We must make a copy of ARG, in case subsequent processing
      alters any part of it.  */
-  arg = break_out_target_exprs (arg);
-
-  /* The use of a default argument has the location of the call, not where it
-     was originally written.  */
-  cp_walk_tree_without_duplicates (&arg, clear_location_r, NULL);
+  arg = break_out_target_exprs (arg, /*clear location*/true);
 
   arg = convert_for_initialization (0, type, arg, LOOKUP_IMPLICIT,
 				    ICR_DEFAULT_ARGUMENT, fn, parmnum,
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 204791e51cf..be77f56fafb 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -7058,7 +7058,7 @@ extern tree build_exception_variant		(tree, tree);
 extern tree bind_template_template_parm		(tree, tree);
 extern tree array_type_nelts_total		(tree);
 extern tree array_type_nelts_top		(tree);
-extern tree break_out_target_exprs		(tree);
+extern tree break_out_target_exprs		(tree, bool = false);
 extern tree build_ctor_subob_ref		(tree, tree, tree);
 extern tree replace_placeholders		(tree, tree, bool * = NULL);
 extern bool find_placeholders			(tree);
diff --git a/gcc/cp/init.c b/gcc/cp/init.c
index e52cd64acc8..5bc8394222b 100644
--- a/gcc/cp/init.c
+++ b/gcc/cp/init.c
@@ -634,7 +634,7 @@ get_nsdmi (tree member, bool in_ctor, tsubst_flags_t complain)
   bool simple_target = (init && SIMPLE_TARGET_EXPR_P (init));
   if (simple_target)
     init = TARGET_EXPR_INITIAL (init);
-  init = break_out_target_exprs (init);
+  init = break_out_target_exprs (init, /*loc*/true);
   if (simple_target && TREE_CODE (init) != CONSTRUCTOR)
     /* Now put it back so C++17 copy elision works.  */
     init = get_target_expr (init);
diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c
index 18e7793b869..dbe84c08a8f 100644
--- a/gcc/cp/tree.c
+++ b/gcc/cp/tree.c
@@ -2908,12 +2908,19 @@ array_type_nelts_total (tree type)
   return sz;
 }
 
+struct bot_data
+{
+  splay_tree target_remap;
+  bool clear_location;
+};
+
 /* Called from break_out_target_exprs via mapcar.  */
 
 static tree
-bot_manip (tree* tp, int* walk_subtrees, void* data)
+bot_manip (tree* tp, int* walk_subtrees, void* data_)
 {
-  splay_tree target_remap = ((splay_tree) data);
+  bot_data &data = *(bot_data*)data_;
+  splay_tree target_remap = data.target_remap;
   tree t = *tp;
 
   if (!TYPE_P (t) && TREE_CONSTANT (t) && !TREE_SIDE_EFFECTS (t))
@@ -2953,7 +2960,8 @@ bot_manip (tree* tp, int* walk_subtrees, void* data)
 			 (splay_tree_key) TREE_OPERAND (t, 0),
 			 (splay_tree_value) TREE_OPERAND (u, 0));
 
-      TREE_OPERAND (u, 1) = break_out_target_exprs (TREE_OPERAND (u, 1));
+      TREE_OPERAND (u, 1) = break_out_target_exprs (TREE_OPERAND (u, 1),
+						    data.clear_location);
       if (TREE_OPERAND (u, 1) == error_mark_node)
 	return error_mark_node;
 
@@ -2993,6 +3001,8 @@ bot_manip (tree* tp, int* walk_subtrees, void* data)
   t = copy_tree_r (tp, walk_subtrees, NULL);
   if (TREE_CODE (*tp) == CALL_EXPR)
     set_flags_from_callee (*tp);
+  if (data.clear_location && EXPR_HAS_LOCATION (*tp))
+    SET_EXPR_LOCATION (*tp, input_location);
   return t;
 }
 
@@ -3001,9 +3011,10 @@ bot_manip (tree* tp, int* walk_subtrees, void* data)
    variables.  */
 
 static tree
-bot_replace (tree* t, int* /*walk_subtrees*/, void* data)
+bot_replace (tree* t, int* /*walk_subtrees*/, void* data_)
 {
-  splay_tree target_remap = ((splay_tree) data);
+  bot_data &data = *(bot_data*)data_;
+  splay_tree target_remap = data.target_remap;
 
   if (VAR_P (*t))
     {
@@ -3041,10 +3052,13 @@ bot_replace (tree* t, int* /*walk_subtrees*/, void* data)
 /* When we parse a default argument expression, we may create
    temporary variables via TARGET_EXPRs.  When we actually use the
    default-argument expression, we make a copy of the expression
-   and replace the temporaries with appropriate local versions.  */
+   and replace the temporaries with appropriate local versions.
+
+   If CLEAR_LOCATION is true, override any EXPR_LOCATION with
+   input_location.  */
 
 tree
-break_out_target_exprs (tree t)
+break_out_target_exprs (tree t, bool clear_location /* = false */)
 {
   static int target_remap_count;
   static splay_tree target_remap;
@@ -3053,9 +3067,10 @@ break_out_target_exprs (tree t)
     target_remap = splay_tree_new (splay_tree_compare_pointers,
 				   /*splay_tree_delete_key_fn=*/NULL,
 				   /*splay_tree_delete_value_fn=*/NULL);
-  if (cp_walk_tree (&t, bot_manip, target_remap, NULL) == error_mark_node)
+  bot_data data = { target_remap, clear_location };
+  if (cp_walk_tree (&t, bot_manip, &data, NULL) == error_mark_node)
     t = error_mark_node;
-  cp_walk_tree (&t, bot_replace, target_remap, NULL);
+  cp_walk_tree (&t, bot_replace, &data, NULL);
 
   if (!--target_remap_count)
     {
diff mbox series

Patch

commit c45f7db420e310b97fca4a83f661efcb6b0fe943
Author: Jason Merrill <jason@redhat.com>
Date:   Mon Apr 9 17:51:15 2018 -0400

            PR debug/65821 - wrong location for main().
    
            * call.c (clear_location_r): New.
            (convert_default_arg): Use it.
            * tree.c (bot_manip): Remove builtin_LINE/FILE handling.

diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index f978ea73f3d..94226d6ea71 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -7296,6 +7296,21 @@  cxx_type_promotes_to (tree type)
   return promote;
 }
 
+/* walk_tree callback to override EXPR_LOCATION in an expression tree.  */
+
+tree
+clear_location_r (tree *tp, int *walk_subtrees, void */*data*/)
+{
+  if (!EXPR_P (*tp))
+    {
+      *walk_subtrees = 0;
+      return NULL_TREE;
+    }
+  if (EXPR_HAS_LOCATION (*tp))
+    SET_EXPR_LOCATION (*tp, input_location);
+  return NULL_TREE;
+}
+
 /* ARG is a default argument expression being passed to a parameter of
    the indicated TYPE, which is a parameter to FN.  PARMNUM is the
    zero-based argument number.  Do any required conversions.  Return
@@ -7360,6 +7375,11 @@  convert_default_arg (tree type, tree arg, tree fn, int parmnum,
   /* We must make a copy of ARG, in case subsequent processing
      alters any part of it.  */
   arg = break_out_target_exprs (arg);
+
+  /* The use of a default argument has the location of the call, not where it
+     was originally written.  */
+  cp_walk_tree_without_duplicates (&arg, clear_location_r, NULL);
+
   arg = convert_for_initialization (0, type, arg, LOOKUP_IMPLICIT,
 				    ICR_DEFAULT_ARGUMENT, fn, parmnum,
 				    complain);
diff --git a/gcc/cp/tree.c b/gcc/cp/tree.c
index d0835cfaa29..18e7793b869 100644
--- a/gcc/cp/tree.c
+++ b/gcc/cp/tree.c
@@ -2992,22 +2992,7 @@  bot_manip (tree* tp, int* walk_subtrees, void* data)
   /* Make a copy of this node.  */
   t = copy_tree_r (tp, walk_subtrees, NULL);
   if (TREE_CODE (*tp) == CALL_EXPR)
-    {
-      set_flags_from_callee (*tp);
-
-      /* builtin_LINE and builtin_FILE get the location where the default
-	 argument is expanded, not where the call was written.  */
-      tree callee = get_callee_fndecl (*tp);
-      if (callee && DECL_BUILT_IN_CLASS (callee) == BUILT_IN_NORMAL)
-	switch (DECL_FUNCTION_CODE (callee))
-	  {
-	  case BUILT_IN_FILE:
-	  case BUILT_IN_LINE:
-	    SET_EXPR_LOCATION (*tp, input_location);
-	  default:
-	    break;
-	  }
-    }
+    set_flags_from_callee (*tp);
   return t;
 }
 
diff --git a/gcc/testsuite/g++.dg/debug/dwarf2/default-arg1.C b/gcc/testsuite/g++.dg/debug/dwarf2/default-arg1.C
new file mode 100644
index 00000000000..d8edffe1a8c
--- /dev/null
+++ b/gcc/testsuite/g++.dg/debug/dwarf2/default-arg1.C
@@ -0,0 +1,14 @@ 
+// PR c++/65821
+// { dg-options "-gdwarf-2 -dA" }
+
+int b = 12;
+
+inline void foo(const int &x = (b+3))
+{
+  b = x;
+}
+
+int main()
+{
+  foo();	      // { dg-final { scan-assembler-not "default-arg1.C:6" } }
+}