Add start to backtrace for debugger #2407

Yuyupo · 2018-06-20T09:54:11Z

With this you can add a start where you want to start listing the backtraces, not just maximum depth.

JerryScript-DCO-1.0-Signed-off-by: Daniella Barsony bella@inf.u-szeged.hu

zherczeg · 2018-06-20T10:01:25Z

jerry-core/debugger/debugger.h

 typedef struct
 {
  uint8_t type; /**< type of the message */
+  uint8_t start; /**< starting point (0 - max_depth)*/


This should be:
uint8_t min_depth[sizeof (uint32_t)]; /**< minimum depth */

Similar to:
uint8_t max_depth[sizeof (uint32_t)]; /**< maximum depth (0 - unlimited) */

Yuyupo · 2018-06-25T06:12:48Z

@zherczeg Changed what was requested

LaszloLango · 2018-06-26T14:12:45Z

jerry-debugger/jerry-client-ws.py

-                max_depth = int(args)
-                if max_depth <= 0:
+                min_depth = int(args[0])
+                max_depth = int(args[1])


What happens if there is args length is 1 and there is no second element?
Both bt max_value and bt min_value max_value commands should work.

I see now, this is intentional and the bt command has two mandatory arguments. IMHO this should be changed. One argument should mean a max_value and two arguments should mean min_value max_value pair.

I don't mind having two mandatory arguments.

Sorry I misunderstood you. Yes, in the python debugger, only one argument must be mandatory. In the message, both values must be sent.

zherczeg · 2018-06-28T07:02:16Z

jerry-debugger/jerry-client-ws.py

-                max_depth = int(args)
-                if max_depth <= 0:
+                min_depth = int(args[0])
+                max_depth = int(args[1])


Sorry I misunderstood you. Yes, in the python debugger, only one argument must be mandatory. In the message, both values must be sent.

zherczeg · 2018-06-28T07:04:08Z

tests/debugger/do_backtrace.expected

 Frame 1: tests/debugger/do_backtrace.js:33 (in test() at line:30, col:1)
 Frame 2: tests/debugger/do_backtrace.js:40
+(jerry-debugger) bt 1 1
+Frame 0: tests/debugger/do_backtrace.js:23 (in foo() at line:21, col:1)


When you specify a min, then maximum (max-min) items could be sent. Here (1-1)==0, so the backtrace should be empty (empty backtrace is valid, you need to test it!!!)

So "bt 1 2" above should only send 1 item. Please also test a large number, e.g. "bt 1000 1001".

LaszloLango · 2018-07-02T13:01:56Z

tests/debugger/do_backtrace.expected

 (jerry-debugger) s
 Stopped at tests/debugger/do_backtrace.js:23 (in foo() at line:21, col:1)
+(jerry-debugger) bt 1 2
+Frame 0: tests/debugger/do_backtrace.js:23 (in foo() at line:21, col:1)


Is this intentional? I thought this should be Frame 1: tests/debugger/do_backtrace.js:33 (in test() at line:30, col:1)

I agree. Hm, this can be confusing. The second number is the count, while the first number is the starting index. So bt 4 4 never returns with anything, since frame index 4 is bigger than the maximum number of frames.

LaszloLango · 2018-07-02T13:03:40Z

tests/debugger/do_backtrace.cmd

 backtrace
+bt 4 4
+bt 600 919
+bt 3 500


What happens if min greater than the max? Can we test it?

Yuyupo · 2018-07-04T07:12:03Z

@LaszloLango Fixed what was requested, and added the test case that was missing.

LaszloLango · 2018-07-04T07:35:18Z

tests/debugger/do_backtrace.expected

 (jerry-debugger) s
 Stopped at tests/debugger/do_backtrace.js:23 (in foo() at line:21, col:1)
+(jerry-debugger) bt 1 2
+Frame 0: tests/debugger/do_backtrace.js:33 (in test() at line:30, col:1)


LaszloLango · 2018-07-04T12:11:46Z

tests/debugger/do_backtrace.expected

 (jerry-debugger) s
 Stopped at tests/debugger/do_backtrace.js:23 (in foo() at line:21, col:1)
+(jerry-debugger) bt 1 2
+Frame 1: tests/debugger/do_backtrace.js:23 (in foo() at line:21, col:1)


the frame index is still wrong

LaszloLango · 2018-07-05T06:30:04Z

jerry-core/debugger/debugger.h

 typedef struct
 {
  uint8_t type; /**< type of the message */
+  uint8_t min_depth[sizeof (uint32_t)]; /**< minimum depth*/


Why don't we use uint16_t for both min_depth and max_depth? @zherczeg, what do you think?

Currently only uint8, cpointer, and uint32 types are used by the protocol. Adding new types would require adding new encoders/decoders for clients. This is certainly possible, but for this it does not seem worth the effort.

I see. Thanks for the explanation.

LaszloLango

LGTM

zherczeg · 2018-07-09T06:58:27Z

jerry-core/debugger/debugger.c

+  if (min_depth <= max_depth)
  {
-    if (frame_ctx_p->bytecode_header_p->status_flags & CBC_CODE_FLAGS_DEBUGGER_IGNORE)
+    uint32_t min_depth_offset = 1;


Why the start depth is 1?

zherczeg · 2018-07-09T07:27:55Z

tests/debugger/do_backtrace.expected

 (jerry-debugger) s
 Stopped at tests/debugger/do_backtrace.js:23 (in foo() at line:21, col:1)
+(jerry-debugger) bt 1 2
+Frame 0: tests/debugger/do_backtrace.js:23 (in foo() at line:21, col:1)


I agree. Hm, this can be confusing. The second number is the count, while the first number is the starting index. So bt 4 4 never returns with anything, since frame index 4 is bigger than the maximum number of frames.

zherczeg

Only one minor thing.

zherczeg · 2018-07-12T03:57:35Z

jerry-debugger/jerry-client-ws.py

+                        print("Error: Positive integer number expected")
+                        return
+                    if self.min_depth > max_depth:
+                        print("Error: Start needs to be lower than depth")


Error: start depth needs to be less equal than max depth

JerryScript-DCO-1.0-Signed-off-by: Daniella Barsony bella@inf.u-szeged.hu

Yuyupo · 2018-07-12T10:33:42Z

@zherczeg Fixed that minor issue

zherczeg

LGTM

zherczeg requested changes Jun 20, 2018

View reviewed changes

Yuyupo force-pushed the backtracestart branch 3 times, most recently from 3e43863 to 2732312 Compare June 22, 2018 12:50

Yuyupo force-pushed the backtracestart branch from 2732312 to 4db1220 Compare June 26, 2018 10:26

LaszloLango reviewed Jun 26, 2018

View reviewed changes

zherczeg requested changes Jun 28, 2018

View reviewed changes

Yuyupo force-pushed the backtracestart branch from 4db1220 to 7b9cf02 Compare June 28, 2018 08:29

LaszloLango reviewed Jul 2, 2018

View reviewed changes

Yuyupo force-pushed the backtracestart branch from 7b9cf02 to 6595198 Compare July 4, 2018 07:10

LaszloLango requested changes Jul 4, 2018

View reviewed changes

Yuyupo force-pushed the backtracestart branch 4 times, most recently from b44eb58 to 829e97f Compare July 4, 2018 10:37

LaszloLango requested changes Jul 4, 2018

View reviewed changes

Yuyupo force-pushed the backtracestart branch from 829e97f to 6651670 Compare July 4, 2018 12:24

LaszloLango reviewed Jul 5, 2018

View reviewed changes

LaszloLango approved these changes Jul 9, 2018

View reviewed changes

zherczeg reviewed Jul 10, 2018

View reviewed changes

Yuyupo force-pushed the backtracestart branch from 6651670 to 2e81a7c Compare July 11, 2018 10:09

zherczeg requested changes Jul 12, 2018

View reviewed changes

Add start to backtrace for debugger

4b826c4

JerryScript-DCO-1.0-Signed-off-by: Daniella Barsony bella@inf.u-szeged.hu

Yuyupo force-pushed the backtracestart branch from 2e81a7c to 4b826c4 Compare July 12, 2018 05:53

zherczeg approved these changes Jul 12, 2018

View reviewed changes

zherczeg merged commit 62cdb39 into jerryscript-project:master Jul 12, 2018

Yuyupo deleted the backtracestart branch May 30, 2019 10:30

Add start to backtrace for debugger #2407

Add start to backtrace for debugger #2407

Uh oh!

Conversation

Yuyupo commented Jun 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Yuyupo commented Jun 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Yuyupo commented Jul 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LaszloLango left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zherczeg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Yuyupo commented Jul 12, 2018

Uh oh!

zherczeg left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants