Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ Add interactive stack trace #233

Open
wants to merge 22 commits into
base: develop
Choose a base branch
from

Conversation

ryan-doan
Copy link

@ryan-doan ryan-doan commented Sep 30, 2022

Summary:

Stack trace now comes with detailed description of where the program fails. This is also saved in a text file in pros-cli/pros/serial/stack_trace.txt

Motivation:

This change help ease the debug of seg faults.

References (optional):

Issue #76

Test Plan:

The code should be tested with stack traces of varied address count, and compared with output from addr2line with hot package, cold package, and monolith to make sure it doesn't omit important information (the algorithm displays addr2line output that contains the character '?' as "??", since these output are most of the time gibberish that doesn't aid debugging, but I'm not sure if this is the case ONLY for gibberish output, which can lead to the algorithm omitting useful debug information).

  • test item

Stack trace now comes with an indicator of which line crashes the program. This is formatted as "cold.package : hot.package : monolith". This trace is also saved to a .txt file
This is where I previously had the code for ease of testing. Deleting it now that I've move the code to terminal.py.
pros/cli/terminal.py Outdated Show resolved Hide resolved
pros/serial/terminal/terminal.py Outdated Show resolved Hide resolved
pros/serial/terminal/terminal.py Outdated Show resolved Hide resolved
test-project2/.gitignore Outdated Show resolved Hide resolved
@Richard-Stump
Copy link

One thing I noticed when looking through the code is that the paths in the code use \, which is a windows only thing. The paths should be changed to use / to be platform independent.

@ryan-doan ryan-doan changed the title Add interactive stack trace ✨ Add interactive stack trace Oct 18, 2022
ryan-doan and others added 3 commits October 18, 2022 21:39
The previous algorithm does not work with how terminal.py prints the stack trace. This algorithm takes it into account, and has been verified to work with a VEX brain.
import sys
import threading

import colorama
from sqlalchemy import null
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
from sqlalchemy import null

class Terminal(object):
"""This class is loosely based off of the pyserial miniterm"""
beginStackTrace = False
stackTraceFile = null
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
stackTraceFile = null
stackTraceFile = None


if "END OF TRACE" in text:
file.close()
file = null
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
file = null
file = None

@@ -29,6 +30,9 @@ def terminal_cli():
help='Specify 2 ports for the "share" backend. The default option deterministically selects ports '
'based on the serial port name')
@click.option('--banner/--no-banner', 'request_banner', default=True)
@click.option('--auto_stack_trace', default=True)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@click.option('--auto_stack_trace', default=True)
@click.option('--raw_stack_trace', is_flag=True, default=False)

@@ -80,11 +84,36 @@ def terminal(port: str, backend: str, **kwargs):
device = devices.RawStreamDevice(ser)
else:
device = devices.vex.V5UserDevice(ser)
term = Terminal(device, request_banner=kwargs.pop('request_banner', True))
term = Terminal(device, request_banner=kwargs.pop('request_banner', True), auto_stack_trace=kwargs.pop('auto_stack_trace', True))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
term = Terminal(device, request_banner=kwargs.pop('request_banner', True), auto_stack_trace=kwargs.pop('auto_stack_trace', True))
term = Terminal(device, request_banner=kwargs.pop('request_banner', True), auto_stack_trace=(not kwargs.pop('auto_stack_trace', False)))

class Terminal(object):
"""This class is loosely based off of the pyserial miniterm"""
beginStackTrace = False
stackTraceFile = None

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
file = None

Did we ever decide if we needed to do this @ryan-doan ?

@BennyBot
Copy link
Member

Ignore all of the suggestions that say "Outdated". Not sure what Github just did.

@omegaStag
Copy link
Contributor

#242

@ayushuk ayushuk added feature A brand new feature in progress This is currently being worked on p: normal Normal priority labels Nov 8, 2022
Fixes a fileNotFound error
@Rocky14683
Copy link
Member

Progress:
截圖 2024-07-23 上午1 23 13
Any thought on indentation and colors?

@WillXuCodes
Copy link
Member

Progress: 截圖 2024-07-23 上午1 23 13 Any thought on indentation and colors?

Wonder how this would look on a small window (like the VSCode integrated terminal).

One other interesting thought, get rid of the "Registers at Exception?" section? We might want to consider shaving some of the fat that your average user may not need. Most important part is the stack trace section. (I'm fully aware addr2line provides all this data, not feasible how this is since it's been so long 💀 )

@BennyBot
Copy link
Member

I think it looks really good. One thing that needs to be done is identifying the deepest level of user code where the data abort comes from (if the user is using hot/cold package, if we have to use monolith to fill in the information we can just ignore this part). However, it would be cool if we could identify this deepest level of user code, somehow get that to VSCode, and then VSCodecan direct the user to that line of code automatically. It may even be possible to guide them through the whole trace in VSCode, perhaps a new "Data Abort Debugger" button on the sidebar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature A brand new feature in progress This is currently being worked on p: normal Normal priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants