This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB 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] -stack-info-depth should always return a count.


Hi,

Thank you for the patch. A few comments below.

On 03/27/2014 06:20 AM, Andrew Burgess wrote:

I believe that the current behaviour would be better if gdb always
returned a depth number, indicating the number of frames that are
available before the stack corruption, the patch below does this.

Out of curiosity, how does -stack-list-frames behave in this scenario? IMO both these commands should offer complimentary data, i.e., if -stack-list-frames outputs N frames before an error, then -stack-info-depth should return N. If -stack-list-frames outputs no frames, then -stack-info-depth should return zero.

If, as I suspect, -stack-list-frames and -stack-info-depth output different numbers of frames for this situation, then I agree. This patch is needed.

If not, well, I don't see any harm, but I have no idea if this kind of change might cause problems for current MI consumers. [To be honest, I'm not sure what value -stack-info-depth has to begin with.] A global/MI maintainer will have to give a recommendation in that case.

diff --git a/gdb/mi/mi-cmd-stack.c b/gdb/mi/mi-cmd-stack.c
index 7bc8114..9a4eec7 100644
--- a/gdb/mi/mi-cmd-stack.c
+++ b/gdb/mi/mi-cmd-stack.c
@@ -170,8 +170,10 @@ void
  mi_cmd_stack_info_depth (char *command, char **argv, int argc)
  {
    int frame_high;
-  int i;
+  int i = 0;
    struct frame_info *fi;
+  volatile struct gdb_exception except;
+  const char *reason;

    if (argc > 1)
      error (_("-stack-info-depth: Usage: [MAX_DEPTH]"));
@@ -183,12 +185,29 @@ mi_cmd_stack_info_depth (char *command, char **argv, int argc)
         the stack.  */
      frame_high = -1;

-  for (i = 0, fi = get_current_frame ();
-       fi && (i < frame_high || frame_high == -1);
-       i++, fi = get_prev_frame (fi))
-    QUIT;
+  TRY_CATCH (except, RETURN_MASK_ERROR)
+    {
+      for (i = 0, fi = get_current_frame ();
+	   fi && (i < frame_high || frame_high == -1);
+	   i++, fi = get_prev_frame (fi))
+	QUIT;
+    }
+
+  if (except.message)

Maintainers recently agreed to do explicit checks for NULL. [if (except.message != NULL) ...]

+    {
+      /* Due to the lazy way state is fetched from the target, if we have
+	 an exception our depth count will be one greater than it should
+	 be.  */
+      reason = except.message;
+      --i;
+    }
+  else if (frame_high == -1 || i < frame_high)
+    reason = _("All frames");
+  else
+    reason = _("Reached frame limit");

    ui_out_field_int (current_uiout, "depth", i);
+  ui_out_field_string (current_uiout, "reason", reason);
  }

  /* Print a list of the locals for the current frame.  With argument of
  2014-03-26  Yao Qi  <yao@codesourcery.com>

  	* lib/gdb.exp (readline_is_used): New proc.

I presume this is a cut-n-paste-o...

diff --git a/gdb/testsuite/gdb.mi/mi-stack.exp b/gdb/testsuite/gdb.mi/mi-stack.exp
index ac7db7a..4977342 100644
--- a/gdb/testsuite/gdb.mi/mi-stack.exp
+++ b/gdb/testsuite/gdb.mi/mi-stack.exp
@@ -137,15 +137,15 @@ proc test_stack_info_depth {} {
      # -stack-info-depth 99

      mi_gdb_test "231-stack-info-depth" \
-	    "231\\^done,depth=\"5\"" \
+        "231\\^done,depth=\"5\",reason=\"\[^\"\]+\"" \
                  "stack info-depth"

      mi_gdb_test "231-stack-info-depth 3" \
-	    "231\\^done,depth=\"3\"" \
+	    "231\\^done,depth=\"3\",reason=\"\[^\"\]+\"" \
                  "stack info-depth 3"

      mi_gdb_test "231-stack-info-depth 99" \
-	    "231\\^done,depth=\"5\"" \
+	    "231\\^done,depth=\"5\",reason=\"\[^\"\]+\"" \
                  "stack info-depth 99"

      mi_gdb_test "231-stack-info-depth 99 99" \


Is there any reason to omit the actual reason="..." in these tests? I don't see where the reason="..." is tested otherwise. I think this necessary. I'm a ever-growing fan of coverage testing.

Is there any way to reliably test the error case/reason="<some error message>"?

Keith


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