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 the buffer argument from loc_getenv #58

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

xiaoxiang781216
Copy link
Contributor

@xiaoxiang781216 xiaoxiang781216 commented Apr 15, 2021

1.Avoid to allocate the big buffer on the caller stack
2.The temporary buffer is only used on cygwin

@xiaoxiang781216
Copy link
Contributor Author

@j256 we are porting dmalloc to RTOS environment, it's critical to reduce the stack consumption as much as possbile.

@j256
Copy link
Owner

j256 commented Apr 15, 2021

Thanks for this. Can you instead make the change where it is used. So revert the changes to loc_compat.[ch] and user_malloc.[ch] and make the dmalloc_t.c, dmalloc.c, dmalloc_argv.c, and user_malloc.c define it statically? Then it is up to the caller to ensure that we are in single thread mode.

@xiaoxiang781216
Copy link
Contributor Author

xiaoxiang781216 commented Apr 17, 2021

Since the buffer is only needed for cygwin, it isn't good to expose the buffer allocation outside loc_getenv to pollute other platform. I I add __thread to make buf become per thread, could you take a look?

@j256
Copy link
Owner

j256 commented Apr 17, 2021

Don't think of portability as polluting. The __thread feels a lot less portable.

@xiaoxiang781216
Copy link
Contributor Author

xiaoxiang781216 commented Apr 17, 2021

__thread is only used in cygwin, and I test that cygwin's clang and gcc both support __thread. So the portability isn't problem here.

@xiaoxiang781216
Copy link
Contributor Author

@j256 I fix the problem by macro to avoid __thread, please review again.

@xiaoxiang781216
Copy link
Contributor Author

@j256 could you take time to review my patch?

1.Avoid to allocate the big buffer on the caller stack
2.The temporary buffer is only used on cygwin

Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
Change-Id: I8bd325624a2f9b8cd1f791f0c07fc76d02158e1d
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.

None yet

2 participants