Closed Bug 1253872 Opened 4 years ago Closed 4 years ago

Make jstests.py to handle --debugger=lldb.

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox47 --- affected
firefox48 --- fixed

People

(Reporter: arai, Assigned: sasacocic, Mentored)

Details

Attachments

(1 file, 4 obsolete files)

Currently jstests.py [1] accepts the command line for debugger with "--debugger" option.
Then, we have to pass --debugger="lldb --" instead of --debugger=lldb in order to debug under lldb.
That's not intuitive, and that doesn't match to the behaviro of jit-test [2], that automatically sets debug command to ['lldb', '--'] when I specify --debugger=lldb

So, it should be better make jstests to handle --debugger=lldb option.


Required code change is following:
  * change jstests.py [3] to set up debugger_prefix variable to ['lldb', '--'] when --debugger=lldb option is specified

To test:
  run jstests [5] with -g --debugger=lldb options, with following command:

    cd js/src/tests/
    ./jstests.py PATH_TO_OBJ_DIR/dist/bin/js ecma_6/Number/ToNumber.js -g --debugger=lldb

  and then run following command in lldb prompt:

    run


You'll need basic knowledge of Python and maybe some of lldb, and be able to build and test SpiderMonkey [4].

A skilled first-time contributor should be able to finish this in two weeks. Leave comments / questions here, or ask me (:arai) in IRC #introduction and #seamonkey channels.

[1] https://dxr.mozilla.org/mozilla-central/source/js/src/tests/jstests.py
[2] https://dxr.mozilla.org/mozilla-central/rev/5a2e0878d6c258b36b0ee8712a2afcde6ad94c78/js/src/jit-test/jit_test.py#283
[3] https://dxr.mozilla.org/mozilla-central/rev/5a2e0878d6c258b36b0ee8712a2afcde6ad94c78/js/src/tests/jstests.py#171
[4] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation
[5] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation#Test
oops, I meant #jsapi channel, not #seamonkey
I would like to work on this bug I think it would be a good fit for me!
thank you :D
feel free to post any question here if you have.
I think i have made the proper corrections

i've run both commands 

cd js/src/tests/./jstests.py PATH_TO_OBJ_DIR/dist/bin/js ecma_6/Number/ToNumber.js -g --debugger=lldb
cd js/src/tests/./jstests.py PATH_TO_OBJ_DIR/dist/bin/js ecma_6/Number/ToNumber.js -g --debugger="lldb --"

and both work properly what should the next step be?
Attached patch bug1253872_lldbDebugFix.diff (obsolete) — Splinter Review
Attachment #8728098 - Flags: review?(arai.unmht)
Comment on attachment 8728098 [details] [diff] [review]
bug1253872_lldbDebugFix.diff

Review of attachment 8728098 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for your patch! :D
It looks good, please address following comment (mostly cosmetic change for readability/maintainability) and ask review again.

Also, please add "Bug 1253872: " before commit message [1], to follow the rule for checkin comment.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Committing_Rules_and_Responsibilities#Checkin_comment

::: js/src/tests/jstests.py
@@ +175,5 @@
>              debugger_prefix.append('--dsymutil=yes')
>          options.show_output = True
>      if options.rr:
>          debugger_prefix = ['rr', 'record']
> +    if options.debugger == 'lldb':

The case "options.debug is truthy" is already handled in following code:
> debugger_prefix = options.debugger.split() if options.debug else []

so it would be better merging the |options.debugger == 'lldb':| case with it and keeping all code realted to |options.debugger| at single place, for readability, and maintainability for further addition.

something similar to the code in jit_test.py:
https://dxr.mozilla.org/mozilla-central/rev/5a2e0878d6c258b36b0ee8712a2afcde6ad94c78/js/src/jit-test/jit_test.py#283
>    if options.debugger:
> ...
>        elif options.debugger == 'lldb':
>            debug_cmd = ['lldb', '--']
> ...
>        else:
>            debug_cmd = options.debugger.split()

@@ +176,5 @@
>          options.show_output = True
>      if options.rr:
>          debugger_prefix = ['rr', 'record']
> +    if options.debugger == 'lldb':
> +        debugger_prefix = ['lldb','--']

Please put a white space after ",".
Attachment #8728098 - Flags: review?(arai.unmht) → feedback+
Assignee: nobody → sasacocic
Attached patch bug1253872_lldbDebugFix.diff (obsolete) — Splinter Review
Attachment #8729643 - Flags: review?(arai.unmht)
I uploaded a new file with the new suggested fixes please tell me if they are not sufficient so i can try and make them better :)
Comment on attachment 8729643 [details] [diff] [review]
bug1253872_lldbDebugFix.diff

Review of attachment 8729643 [details] [diff] [review]:
-----------------------------------------------------------------

sorry, I should've been more clear.

it was totally okay to check with |options.debugger == 'lldb'|.
then, if the condition is true, we don't have to do |options.debugger.split()|, at line 171:

  debugger_prefix = options.debugger.split() if options.debug else []

so, first, the line 171 should be changed to normal if-else:

  if options.debug:
      debugger_prefix = options.debugger.split() 
  else:
      debugger_prefix = []

then |options.debugger == 'lldb'| case should be handled in then-clause:

  if options.debug:
      if options.debugger == 'lldb':
          ...
      else:
          debugger_prefix = options.debugger.split() 
  else:
      debugger_prefix = []

with this way, all code related to |options.debugger| is inside single if-else, and it's clear where to change the code next time, like following:

  if options.debug:
      if options.debugger == 'lldb':
          ...
      elif options.debugger == ...:
          ...
      elif options.debugger == ...:
          ...
      else:
          debugger_prefix = options.debugger.split() 
  else:
      debugger_prefix = []

does it make sense?
Attachment #8729643 - Flags: review?(arai.unmht) → feedback+
Attached patch bug1253872_lldbDebugFix.diff (obsolete) — Splinter Review
Attachment #8729741 - Flags: review?(arai.unmht)
ok i made the changes but i wasn't sure about every thing so i did my best. Also should i make sure it works for cases like '--debugger=rr' or '--debugger=gdb' or just make sure it works for '--debugger=lldb'. I'm not sure the others work because I've tried '--debugger=rr' and it didn't work or maybe they work and i'm just not using it correctly.
Comment on attachment 8729741 [details] [diff] [review]
bug1253872_lldbDebugFix.diff

Review of attachment 8729741 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks again!

yeah, the |options.debugger| parts is perfect now :)
but this patch introduces unintentional behavior change for |options.valgrind| and |options.rr|, as they didn't require |options.debug|, but now they do.

::: js/src/tests/jstests.py
@@ +178,5 @@
> +            if os.uname()[0] == 'Darwin':
> +                debugger_prefix.append('--dsymutil=yes')
> +            options.show_output = True
> +        if options.rr:
> +            debugger_prefix = ['rr', 'record']

so, please move |options.valgrind| and |options.rr| cases out of this if-else block, back to original position (I mean, after this if-else block)
Attachment #8729741 - Flags: review?(arai.unmht) → feedback+
(In reply to sasa cocic from comment #12)
> Also should i make sure it works for cases like '--debugger=rr' or
> '--debugger=gdb' or just make sure it works for '--debugger=lldb'. I'm not
> sure the others work because I've tried '--debugger=rr' and it didn't work
> or maybe they work and i'm just not using it correctly.

for now, this bug is only for lldb.
maybe someone will file followup bug for them tho.


if you're interested in rr, here's link
  https://github.com/mozilla/rr
Attached patch bug1253872_lldbDebugFix.diff #4 (obsolete) — Splinter Review
Ok i've updated the file by moving |options.valgrind| and |options.rr|
Attachment #8729751 - Flags: review?(arai.unmht)
Comment on attachment 8729751 [details] [diff] [review]
bug1253872_lldbDebugFix.diff #4

Review of attachment 8729751 [details] [diff] [review]:
-----------------------------------------------------------------

thank you for quick response :)

::: js/src/tests/jstests.py
@@ +173,5 @@
> +            debugger_prefix = ['lldb', '--']
> +        else:
> +            debugger_prefix = options.debugger.split()
> +    else:
> +        options.debugger = []

wow, sorry I overlooked this one.
it should assign to |debugger_prefix|, not |options.debugger|.

almost there :)
Attachment #8729751 - Flags: review?(arai.unmht) → feedback+
OK! there we go thats fixed. So whats next then?
Attachment #8729762 - Flags: review?(arai.unmht)
Comment on attachment 8729762 [details] [diff] [review]
bug1253872_lldbDebugFix.diff

Review of attachment 8729762 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent! thank you for your work! :D

Here's try run (automated test) for your patch.
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=738e713fd53a

Once all tests there run successfully (or only with known failure), this patch can be landed to the tree.
it may take 2 or 3 hours until all tests finish.
Attachment #8729762 - Flags: review?(arai.unmht) → review+
Keywords: checkin-needed
Attachment #8728098 - Attachment is obsolete: true
Attachment #8729643 - Attachment is obsolete: true
Attachment #8729741 - Attachment is obsolete: true
Comment on attachment 8729751 [details] [diff] [review]
bug1253872_lldbDebugFix.diff #4

In the future, please mark old patches as obsolete when you're attaching updated ones. Thanks!
Attachment #8729751 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/bc7045b5a4d5
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.