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

Remove binaryEdit::deleteBinaryEdit #866

Merged
merged 1 commit into from
Sep 29, 2020
Merged

Remove binaryEdit::deleteBinaryEdit #866

merged 1 commit into from
Sep 29, 2020

Conversation

hainest
Copy link
Contributor

@hainest hainest commented Sep 25, 2020

This moves the same functionality into the destructor and prevents possible memory corruption when its usage would promote in-place object reuse. Originally part of #317.

This moves the same functionality into the destructor and prevents possible memory corruption when its usage would promote in-place object reuse.
@hainest hainest added code cleanup Bring the code up to modern standards or remove deprecated features API-BREAKER This change breaks the public API labels Sep 25, 2020
@hainest hainest added this to the Release 11.0 milestone Sep 25, 2020
@hainest hainest self-assigned this Sep 25, 2020
@hainest hainest removed the API-BREAKER This change breaks the public API label Sep 29, 2020
@hainest hainest removed this from the Release 11.0 milestone Sep 29, 2020
@hainest
Copy link
Contributor Author

hainest commented Sep 29, 2020

https://bottle.cs.wisc.edu/search?dyninst_branch=PR866

No new regressions (the ones on ulna are unrelated).

@hainest
Copy link
Contributor Author

hainest commented Sep 29, 2020

@n738cu @mxz297 @sashanicolas @kupsch @bigtrak

We have generally considered the internal headers (e.g., dyninstAPI/src/binaryEdit.h) to not be visible to the user; these headers are never exported to dyninst_install/include during installation. However, the binaryEdit type is leaked through BPatch_binaryEdit::lowlevel_edit() which is visible to the user. There are many such members in user-facing types that expose the underlying Dyninst interfaces that we actually don't want to be public. Should we just ignore this and continue to treat these internal interfaces as mutable with a big CAVEAT EMPTOR to the user of something like BPatch_binaryEdit::ll_edit(), or do we want to gradually break these leaked internal APIs?

@hainest hainest merged commit 07b506e into dyninst:api_breakages Sep 29, 2020
@hainest hainest deleted the remove_binaryEdit_deleteBinaryEdit branch September 29, 2020 20:09
@hainest hainest restored the remove_binaryEdit_deleteBinaryEdit branch September 29, 2020 20:10
@hainest hainest deleted the remove_binaryEdit_deleteBinaryEdit branch September 29, 2020 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code cleanup Bring the code up to modern standards or remove deprecated features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant