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.

Sign up for GitHub

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

Jump to bottom

Fix external pagemap usage. #221

Merged
merged 4 commits into from
Jan 8, 2021
Merged

Fix external pagemap usage. #221

merged 4 commits into from
Jan 8, 2021

Conversation

davidchisnall
Copy link
Collaborator

At some point in the refactoring, these were broken.

@davidchisnall davidchisnall requested a review from mjp41 June 30, 2020 10:40
@mjp41
Copy link
Member

mjp41 commented Jun 30, 2020

Could you add unit tests, so these don't break in the future?

@davidchisnall
Copy link
Collaborator Author

I've been pondering how to do this. These only show up when snmalloc is used as the malloc implementation in libc and also in another library.

@mjp41
Copy link
Member

mjp41 commented Jun 30, 2020

So we have tests that build two different allocators, could you modify that to cover this case:
https://github.com/microsoft/snmalloc/tree/master/src/test/func/two_alloc_types

Does it have to be in libc?

@davidchisnall
Copy link
Collaborator Author

Possibly not. I'll have a look at whether we can make one of those find the other's pagemap.

@davidchisnall davidchisnall mentioned this pull request Jul 20, 2020
Closed
@davidchisnall
Copy link
Collaborator Author

I've added a unit test. This is blocking work in Verona, so it would be good to get it merged soon.

mjp41
mjp41 approved these changes Jan 7, 2021
View reviewed changes
Copy link
Member

@mjp41 mjp41 left a comment

Choose a reason for hiding this comment

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

Looks good minor fixes

src/test/func/external_pagemap/external_pagemap.cc Outdated Show resolved Hide resolved
src/test/func/external_pagemap/external_pagemap.cc Outdated Show resolved Hide resolved
@davidchisnall davidchisnall force-pushed the davidchisnall/small-fixes branch from 3cf85dd to 7556de9 Compare January 8, 2021 10:36
mjp41
mjp41 reviewed Jan 8, 2021
View reviewed changes
src/test/func/external_pagemap/external_pagemap.cc
@@ -0,0 +1,30 @@
#if defined(SNMALLOC_PASS_THROUGH) || defined(_WIN32)
Copy link
Member

Choose a reason for hiding this comment

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

If you set a SNMALLOC_NAME_MANGLE then you can make the test work on Windows.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's not the problem. The Windows declarations for the standard malloc functions have a bunch of declspecs in them that we're not using and this causes compile failures.

Copy link
Member

Choose a reason for hiding this comment

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

If you had set a name mangle then they wouldn't conflict.

@davidchisnall davidchisnall merged commit 4837c82 into master Jan 8, 2021
@davidchisnall davidchisnall deleted the davidchisnall/small-fixes branch January 11, 2021 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mjp41 mjp41 mjp41 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@davidchisnall @mjp41