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

fix(skymp5-server): debug crash #2213

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Pospelove
Copy link
Contributor

@Pospelove Pospelove commented Nov 10, 2024

Important

Enhance debugging in MpObjectReference.cpp by tracking property changes and logging detailed information when ApplyChangeForm is called after SetProperty.

  • Debugging Enhancements:
    • Add SetPropertyTrackingInfo struct to track property changes in MpObjectReference.cpp.
    • Capture stack trace in SetProperty() when GetFormId() is 0xE2218.
    • Log detailed information in ApplyChangeForm() if called after SetProperty().
  • Logging:
    • Replace std::terminate() with detailed logging in ApplyChangeForm() to provide more context on errors.

This description was created by Ellipsis for 638e4a0. It will automatically update as commits are pushed.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Reviewed everything up to 638e4a0 in 1 minute and 5 seconds

More details
  • Looked at 85 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. skymp5-server/cpp/server_guest_lib/MpObjectReference.cpp:1175
  • Draft comment:
    Remove the commented-out std::terminate(); line to avoid confusion.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PR modifies the ApplyChangeForm function to log critical information instead of terminating the program. However, the commented-out std::terminate() line should be removed to avoid confusion.
2. skymp5-server/cpp/server_guest_lib/MpObjectReference.cpp:32
  • Draft comment:
    Ensure that <stacktrace> is supported in the build environment or provide an alternative for older C++ standards.
  • Reason this comment was not posted:
    Comment did not seem useful.

Workflow ID: wflow_JRa0iFqT4pN0vLXW


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@@ -1129,8 +1146,33 @@ MpChangeForm MpObjectReference::GetChangeForm() const
void MpObjectReference::ApplyChangeForm(const MpChangeForm& changeForm)
Copy link

Choose a reason for hiding this comment

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

Consider resetting pImpl->setPropertyTrackingInfo after logging in ApplyChangeForm to avoid incorrect logging in subsequent calls.

@nic11
Copy link
Collaborator

nic11 commented Dec 16, 2024

Maybe close? It won't work without either C++23 on (which needs at least a clang update) or cpptrace or similar library added

@Pospelove
Copy link
Contributor Author

@nic11 could u please send a link to a discord topic where we were discussing this.

Also I want to introduce formal rules for closing PRs due to inactivity. CLosing important PRs should always lead to a ticket creation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants