Skip to content

fix: fix shadowstack pass discard debuginfo #2496

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

Merged

Conversation

HerrCai0907
Copy link
Member

In shadowstack pass, if some function need to add temp local, It need to be removed and re-added. It will discard all debug info because debug info bind with functionref.

This PR aims at re-assign the debug info after shadowstack pass

  • I've read the contributing guidelines
  • I've added my name and email to the NOTICE file

@dcodeIO
Copy link
Member

dcodeIO commented Sep 2, 2022

Would it perhaps be preferable to add a replaceFunction(..., preserveDebugInfo = true) helper of sorts that can be called instead of removing the function and adding a new one? I guess then we'd not need to remember intermediate state.

@MaxGraey
Copy link
Member

MaxGraey commented Sep 2, 2022

Yeah, also prefer to update
updateFunction(funcRef: FunctionRef)
to
updateFunction(funcRef: FunctionRef, preserveDebugInfo: bool)

or simply check this.options.debugInfo inside updateFunction

@HerrCai0907
Copy link
Member Author

HerrCai0907 commented Sep 2, 2022

Would it perhaps be preferable to add a replaceFunction(..., preserveDebugInfo = true) helper of sorts that can be called instead of removing the function and adding a new one? I guess then we'd not need to remember intermediate state.

You mean add this API in binaryen? I also agree it is more eleganta

@HerrCai0907 HerrCai0907 requested a review from MaxGraey September 4, 2022 14:24
@MaxGraey MaxGraey merged commit f9dee72 into AssemblyScript:main Sep 25, 2022
@HerrCai0907 HerrCai0907 deleted the fix/incremental-discard-debuginfo branch September 26, 2022 01:34
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.

3 participants