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

Compile lua by C++ compilers to avoid memory leaks #614

Merged
merged 12 commits into from
Jun 1, 2022

Conversation

PragmaTwice
Copy link
Member

@PragmaTwice PragmaTwice commented May 31, 2022

The clause by C++ compilers just means in C++ mode since most mainstream C compilers are also a C++ compiler. 😆
Close #609.

@PragmaTwice
Copy link
Member Author

PragmaTwice commented May 31, 2022

There are some weird problem in the default xcode toolchains (i.e. apple clang):

$ echo "#include <assert.h>" | /Applications/Xcode_13.2.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ -c -
clang: warning: treating 'c' input as 'c++' when in C++ mode, this behavior is deprecated [-Wdeprecated]
a.c:1:10: fatal error: 'assert.h' file not found
#include <assert.h>
         ^~~~~~~~~~
1 error generated.

So I try to use clang++ in PATH as cxx compiler instead.

@PragmaTwice

This comment was marked as outdated.

@PragmaTwice PragmaTwice changed the title Compile lua by C++ compiler to avoid memory leak Compile lua by C++ compilers to avoid memory leaks May 31, 2022
@git-hulk
Copy link
Member

@PragmaTwice Seems ${CMAKE_CXX_COMPILER} will use the apple-clang and $CXX will use clang++, clang++ works good after updating the Lua. So we can replace ${CMAKE_CXX_COMPILER} with ${CXX} in lua.cmake?

@PragmaTwice
Copy link
Member Author

PragmaTwice commented May 31, 2022

@PragmaTwice Seems ${CMAKE_CXX_COMPILER} will use the apple-clang and $CXX will use clang++, clang++ works good after updating the Lua. So we can replace ${CMAKE_CXX_COMPILER} with ${CXX} in lua.cmake?

I think the concern here is that we'd better to keep the same c++ compiler during the whole build process. So I pass the c++ compiler specified by cmake to make.

@git-hulk
Copy link
Member

@PragmaTwice Seems ${CMAKE_CXX_COMPILER} will use the apple-clang and $CXX will use clang++, clang++ works good after updating the Lua. So we can replace ${CMAKE_CXX_COMPILER} with ${CXX} in lua.cmake?

I think the concern here is that we'd better to keep the same compiler during the whole build process. So I pass the c++ compiler specified by cmake to make.

I got your point, but it will fail to build if we didn’t specify them explicitly. I’m not sure if it’s ok to other users.

@PragmaTwice
Copy link
Member Author

PragmaTwice commented May 31, 2022

@PragmaTwice Seems ${CMAKE_CXX_COMPILER} will use the apple-clang and $CXX will use clang++, clang++ works good after updating the Lua. So we can replace ${CMAKE_CXX_COMPILER} with ${CXX} in lua.cmake?

I think the concern here is that we'd better to keep the same compiler during the whole build process. So I pass the c++ compiler specified by cmake to make.

I got your point, but it will fail to build if we didn’t specify them explicitly. I’m not sure if it’s ok to other users.

It is a common usage to specify CMAKE_CXX_COMPILER before build e.g. cmake .. -DCMAKE_CXX_COMPILER=/some/path/some-g++, so I think using CXX may lead to different compilers in one build.
I think we can use try_compile to workaround it: if it is in osx and try_compile(a code snippet with <assert.h>) fail, we will use CXX, otherwise we use CMAKE_CXX_COMPILER to keep C++ compiler unchanged.
These logics are easy to implement in cmake : )

@PragmaTwice
Copy link
Member Author

PragmaTwice commented Jun 1, 2022

It is a common usage to specify CMAKE_CXX_COMPILER before build e.g. cmake .. -DCMAKE_CXX_COMPILER=/some/path/some-g++, so I think using CXX may lead to different compilers in one build. I think we can use try_compile to workaround it: if it is in osx and try_compile(a code snippet with <assert.h>) fail, we will use CXX, otherwise we use CMAKE_CXX_COMPILER to keep C++ compiler unchanged. These logics are easy to implement in cmake : )

done 🎉

@git-hulk
Copy link
Member

git-hulk commented Jun 1, 2022

cool, thanks to @PragmaTwice great work!

@tisonkun
Copy link
Member

tisonkun commented Jun 1, 2022

@PragmaTwice as RocksLabs/lua#3 merged, is it still necessary to introduce such workaround? IIUC now it should be happy to build with CMAKE_CXX_COMPILER.

@PragmaTwice
Copy link
Member Author

PragmaTwice commented Jun 1, 2022

I think I figure out this problem.
CMake deals with apple clang well because it automatically adds -isysroot ${CMAKE_OSX_SYSROOT} to compile options.
But Makefiles in lua do not do that, so assert.h cannot be found while using apple clang++.
We can just add it to LUA_CFLAGS!
@git-hulk @tisonkun

@PragmaTwice
Copy link
Member Author

PragmaTwice commented Jun 1, 2022

Sorry for many time to solve it, I do not have an Mac osx device so I use github actions in a temp repo to debug the build process.

Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

Great work! Thanks for your contribution.

@git-hulk git-hulk merged commit b9f7863 into apache:unstable Jun 1, 2022
@git-hulk
Copy link
Member

git-hulk commented Jun 1, 2022

Thanks for @PragmaTwice great investigation and contribution again. 👍

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.

Memory leaks in Lua::redisGenericCommand
3 participants