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

Fixed memory copy error of virtual table under multiple inheritance #1695

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

Conversation

hxbb00
Copy link
Contributor

@hxbb00 hxbb00 commented Nov 18, 2022

No description provided.

@josetr
Copy link
Contributor

josetr commented Nov 18, 2022

Can you add a test please?

@hxbb00
Copy link
Contributor Author

hxbb00 commented Nov 18, 2022

I have passed the test in the project of my company. Sorry, the project environment of my company is too big, so it is not suitable to provide the test environment. My company used this in the project, I use it in practice, so it shouldn't be a problem!

@hxbb00
Copy link
Contributor Author

hxbb00 commented Nov 18, 2022

image
this is mycase,here the __Internal:
image

@hxbb00
Copy link
Contributor Author

hxbb00 commented Nov 18, 2022

image
The difference with this modification is that there is no problem when the offset is 0, but when the offset is not 0, the address is calculated incorrectly

@josetr
Copy link
Contributor

josetr commented Nov 18, 2022

My company used this in the project, I use it in practice, so it shouldn't be a problem!

This only tells us that it works for your specific use case and your specific platform / ABI. We have to support multiple ABI's. You may be fixing the issue in one platform and then breaking the rest.

You have made a change to this line before without a test and you are doing it again. Your previous PR shouldn't have been merged.

@tritao
Copy link
Collaborator

tritao commented Nov 18, 2022

@hxbb00 We really appreciate your contribution but as @josetr said it would be great if you could add a test.

Can you try to recreate this class hierarchy in https://github.com/mono/CppSharp/blob/main/tests/VTables/VTables.h for example?

@hxbb00 hxbb00 closed this Nov 23, 2022
@tritao tritao reopened this Nov 23, 2022
@tritao
Copy link
Collaborator

tritao commented Nov 23, 2022

Leaving this open for now so we don't forget about it

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