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

Factor out libc code into a header. #624

Merged
merged 6 commits into from
Aug 9, 2023
Merged

Factor out libc code into a header. #624

merged 6 commits into from
Aug 9, 2023

Conversation

mjp41
Copy link
Member

@mjp41 mjp41 commented Jul 20, 2023

This pulls the main definitions of the various libc malloc functions into a header for easier use and inclusion in other projects.

This pulls the main definitions of the various libc malloc functions
into a header for easier use and inclusion in other projects.
@davidchisnall
Copy link
Collaborator

Do they still get inlined in the .cc file?

@mjp41
Copy link
Member Author

mjp41 commented Jul 20, 2023

It looks good:

00000000000054e0 g     F .text  00000000000000b9              free
00000000000054e0 g     F .text  00000000000000b9              cfree
00000000000054e0 g     F .text  00000000000000b9              operator delete(void*)
00000000000054e0 g     F .text  00000000000000b9              operator delete[](void*)
00000000000054e0 g     F .text  00000000000000b9              operator delete(void*, std::align_val_t)
00000000000054e0 g     F .text  00000000000000b9              operator delete[](void*, std::align_val_t)

0000000000005430 g     F .text  00000000000000a8              malloc
0000000000005430 g     F .text  00000000000000a8              operator new(unsigned long)
0000000000005430 g     F .text  00000000000000a8              operator new[](unsigned long)
0000000000005ed0 g     F .text  00000000000000b6              operator new(unsigned long, std::align_val_t)
0000000000005ed0 g     F .text  00000000000000b6              operator new[](unsigned long, std::align_val_t)

and

0000000000005430 <operator new[](unsigned long)>:
    5430:       48 8d 47 ff             lea    -0x1(%rdi),%rax
    5434:       48 3d ff df 00 00       cmp    $0xdfff,%rax
    543a:       77 3a                   ja     5476 <operator new[](unsigned long)+0x46>
    543c:       48 c1 e8 04             shr    $0x4,%rax
    5440:       48 8d 0d 51 cc ff ff    lea    -0x33af(%rip),%rcx        # 2098 <snmalloc::sizeclass_lookup>
    5447:       0f b6 14 08             movzbl (%rax,%rcx,1),%edx
    544b:       48 8b 35 e6 e0 00 00    mov    0xe0e6(%rip),%rsi        # 13538 <_DYNAMIC+0x1e0>
    5452:       64 48 8b 04 25 00 00    mov    %fs:0x0,%rax
    5459:       00 00
    545b:       48 01 f0                add    %rsi,%rax
    545e:       48 8d 0c d0             lea    (%rax,%rdx,8),%rcx
    5462:       64 48 8b 04 d6          mov    %fs:(%rsi,%rdx,8),%rax
    5467:       48 85 c0                test   %rax,%rax
    546a:       74 22                   je     548e <operator new[](unsigned long)+0x5e>
    546c:       48 8b 10                mov    (%rax),%rdx
    546f:       0f 18 0a                prefetcht0 (%rdx)
    5472:       48 89 11                mov    %rdx,(%rcx)
    5475:       c3                      ret
    ....

@mjp41 mjp41 requested a review from nwf-msr July 31, 2023 14:57
Copy link
Contributor

@nwf-msr nwf-msr left a comment

Choose a reason for hiding this comment

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

Two hopefully quick questions, but the bulk of it seems fine.

src/snmalloc/mem/localalloc.h Show resolved Hide resolved
* Clang was helpfully inlining the constant return value, and
* thus converting from a tail call to an ordinary call.
*/
static inline void* snmalloc_not_allocated = nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does marking this const re-enable the problematic inlining?

Copy link
Member Author

Choose a reason for hiding this comment

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

I had a look at the code a bit more, and it no longer seems like it is a problem.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also ended up adding some SNMALLOC_FAST_PATH to force some inlining as calloc and realloc had introduced an additional tail call than needed.

Copy link
Contributor

@nwf-msr nwf-msr left a comment

Choose a reason for hiding this comment

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

LGTM

@mjp41 mjp41 merged commit 6cbc50f into microsoft:main Aug 9, 2023
@mjp41 mjp41 deleted the libc_api branch August 9, 2023 06:15
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