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

Compiling mruby as C++ code #3267

Closed
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@dabroz
Contributor

dabroz commented Nov 23, 2016

This is an artifact discovered during #3258.

mruby offers a way to use C++ exceptions instead of C jumps. This is a great idea, especially given that C++ exceptions cost nothing on modern compilers.

However, this is made in such a way that most of mruby code is compiled as C, and only some special files (vm.c, error.c, etc.) are complied as C++, while still being defined as "extern C".

This is unfortunately an undefined behavior, and many compilers won't generate proper exception propagation code in functions defined as such. That includes both GCC and Clang on different platforms.

The proper solution is to compile all of mruby code as C++. This pull request deals with this issue in a following manner:

  • fixed few issues with code that was proper C but not C++ (ie. implicit void* casts)
  • added macros for C-style definitions like INT32_MAX, SIZE_MAX, etc.
  • added export declarations to MRBC outputs
  • changed the method of compiling mruby with C++ ABI to use -x c++

The last point should be reworked in the future (hacks for marking some files as C++ would no longer be needed), but it serves well for now, and increases the compatibility of mruby significantly.

matz added a commit that referenced this pull request Nov 24, 2016

@matz

This comment has been minimized.

Show comment
Hide comment
@matz

matz Nov 24, 2016

Member

PR merged. Thank you!

Member

matz commented Nov 24, 2016

PR merged. Thank you!

@matz matz closed this Nov 24, 2016

@dabroz dabroz deleted the dabroz:feature-mrubycxx branch Nov 25, 2016

@kjunichi

This comment has been minimized.

Show comment
Hide comment
@kjunichi

kjunichi Dec 28, 2016

Contributor

これ、なんで取り込んだんですか?
既存のC++とCが混在しているmrbgemでリンクエラーになるんですけど。。

Why did you incorporate this?
I get a link error with mrbgem where existing C ++ and C are mixed. .

Contributor

kjunichi commented Dec 28, 2016

これ、なんで取り込んだんですか?
既存のC++とCが混在しているmrbgemでリンクエラーになるんですけど。。

Why did you incorporate this?
I get a link error with mrbgem where existing C ++ and C are mixed. .

@matz

This comment has been minimized.

Show comment
Hide comment
@matz

matz Dec 28, 2016

Member

Ah, actually I thought it improve C++ linkage. If it makes handling C++ mrbgem worse, I will revert the PR.

Member

matz commented Dec 28, 2016

Ah, actually I thought it improve C++ linkage. If it makes handling C++ mrbgem worse, I will revert the PR.

@kjunichi

This comment has been minimized.

Show comment
Hide comment
@kjunichi

kjunichi Dec 28, 2016

Contributor

I'd like to use C ++ exceptions conveniently, but I would like you to revert.

That's for the following two reasons.

  • The author of mrbgem using existing C ++ needs to modify the declaration of extern "C".
  • If you want to put mrbgem using C ++, all other mrbgem written in C also needs to be C ++ compatible code.(As well as the modifications in the C code of the mruby itself in this patch)
Contributor

kjunichi commented Dec 28, 2016

I'd like to use C ++ exceptions conveniently, but I would like you to revert.

That's for the following two reasons.

  • The author of mrbgem using existing C ++ needs to modify the declaration of extern "C".
  • If you want to put mrbgem using C ++, all other mrbgem written in C also needs to be C ++ compatible code.(As well as the modifications in the C code of the mruby itself in this patch)
@dabroz

This comment has been minimized.

Show comment
Hide comment
@dabroz

dabroz Dec 28, 2016

Contributor

You don't need to use extern "C". Mruby has a macro MRB_BEGIN_DECL that adds an appropriate block if needed. Other than that you only need to ensure that the gem's code is also valid C++ - but that is usually simple to do (sometimes need to add type casts).

Without this patch, mruby is broken on many platforms.

You can also use mruby in C++ without using C++ exceptions, in such scenario mruby and gems' code would be compiled as C code.

Contributor

dabroz commented Dec 28, 2016

You don't need to use extern "C". Mruby has a macro MRB_BEGIN_DECL that adds an appropriate block if needed. Other than that you only need to ensure that the gem's code is also valid C++ - but that is usually simple to do (sometimes need to add type casts).

Without this patch, mruby is broken on many platforms.

You can also use mruby in C++ without using C++ exceptions, in such scenario mruby and gems' code would be compiled as C code.

@kjunichi

This comment has been minimized.

Show comment
Hide comment
@kjunichi

kjunichi Dec 28, 2016

Contributor

I already made mrbgems .your patch makes me change my code!

Contributor

kjunichi commented Dec 28, 2016

I already made mrbgems .your patch makes me change my code!

@@ -6,6 +6,7 @@ MRuby::Toolchain.new(:gcc) do |conf, _params|
cc.option_include_path = '-I%s'
cc.option_define = '-D%s'
cc.compile_options = '%{flags} -MMD -o %{outfile} -c %{infile}'
cc.cxx_compile_flag = '-x c++ -std=c++03'

This comment has been minimized.

@zzak

zzak Dec 28, 2016

Member

I guess this change makes this flag required now?

Is there some way to do this in a backwards compatible way? i.e. default flag which supports both versions

@zzak

zzak Dec 28, 2016

Member

I guess this change makes this flag required now?

Is there some way to do this in a backwards compatible way? i.e. default flag which supports both versions

@dabroz

This comment has been minimized.

Show comment
Hide comment
@dabroz

dabroz Dec 28, 2016

Contributor

@kjunichi If you used previous version of mixed C/C++ mruby, and had C++ exceptions cross the language barrier (which is likely if you mix Ruby with C(++)), then you get undefined behavior. Some compilers permit it, but other compilers enforce it, and will result in segmentation fault in such case.

I'm not saying that the current handling of C/C++ mode is perfect, and likely should be refactored, but we need to choose a correct (in terms of language correctness) solution; even if it would require small code adjustments for gem authors.

Contributor

dabroz commented Dec 28, 2016

@kjunichi If you used previous version of mixed C/C++ mruby, and had C++ exceptions cross the language barrier (which is likely if you mix Ruby with C(++)), then you get undefined behavior. Some compilers permit it, but other compilers enforce it, and will result in segmentation fault in such case.

I'm not saying that the current handling of C/C++ mode is perfect, and likely should be refactored, but we need to choose a correct (in terms of language correctness) solution; even if it would require small code adjustments for gem authors.

@kjunichi

This comment has been minimized.

Show comment
Hide comment
@kjunichi

kjunichi Dec 28, 2016

Contributor

@dabroz

I already ran several mrbgems using multiple C ++ on multiple platforms such as Linux, macOS, windows without crashing.
From this experience I was unaware of the "undefined behavior" you said here.
This is because I have not written any code using any C ++ exception.

and had C++ exceptions

Now I understood.

I think we cannot stop using your patch even though there is fix for C only gem.

thanks for taking your time.

Contributor

kjunichi commented Dec 28, 2016

@dabroz

I already ran several mrbgems using multiple C ++ on multiple platforms such as Linux, macOS, windows without crashing.
From this experience I was unaware of the "undefined behavior" you said here.
This is because I have not written any code using any C ++ exception.

and had C++ exceptions

Now I understood.

I think we cannot stop using your patch even though there is fix for C only gem.

thanks for taking your time.

@matz

This comment has been minimized.

Show comment
Hide comment
@matz

matz Dec 30, 2016

Member

@dabroz could you elaborate the situation this PR fixes? What kind of problems on which platforms?
By seeing the info, we may be able to make a new workaround.

Member

matz commented Dec 30, 2016

@dabroz could you elaborate the situation this PR fixes? What kind of problems on which platforms?
By seeing the info, we may be able to make a new workaround.

@dabroz

This comment has been minimized.

Show comment
Hide comment
@dabroz

dabroz Jan 4, 2017

Contributor

@matz: Sure.

Let's use following program, build with 2 source files. One of them is C, the other one is C++. To be able to use each others functions, C++ functions are declared as extern "C".

This is of course a simplified version of mruby code, where (before this patch) most of the codebase was C, and exception-related parts were C++ with C linkage.

// cpp_code.cpp
#include <stdio.h>

extern "C" void c_func();

extern "C" void cpp_func()
{
	throw 1;
}

int main()
{
	try
	{
		c_func();
	}
	catch (int exception)
	{
		fprintf(stderr, "Exception correctly caught: %d\n", exception);
	}
}
// c_code.c
void cpp_func();

void c_func()
{
	cpp_func();
}

Let's build them like this:

clang++ ${FLAGS} -c cpp_code.cpp -o cpp_code.o
clang   ${FLAGS} -c c_code.c     -o c_code.o
clang++ ${FLAGS} c_code.o cpp_code.o -o test
./test

Now, everything depends on FLAGS. If we run this with empty FLAGS, the compiler will default to 64-bit mode, and the code will run correctly:

Exception correctly caught: 1

However, if we set FLAGS to -m32, forcing 32-bit mode, the result will be very different:

libc++abi.dylib: terminating with uncaught exception of type int

The problem observable here is undefined behavior of C++ exceptions passing C code barrier. It may run correctly if the ABI supports is (which is the case under 64-bit OSX), but it doesn't need to (like 32-bit OSX) - and then the code will crash as soon as the exception crosses C/C++ barrier.

I am running these tests on OSX (10.12.2) with Apple LLVM version 8.0.0 (clang-800.0.42.1), but the same behavior is visible under other versions of Clang and also under Linux.

Contributor

dabroz commented Jan 4, 2017

@matz: Sure.

Let's use following program, build with 2 source files. One of them is C, the other one is C++. To be able to use each others functions, C++ functions are declared as extern "C".

This is of course a simplified version of mruby code, where (before this patch) most of the codebase was C, and exception-related parts were C++ with C linkage.

// cpp_code.cpp
#include <stdio.h>

extern "C" void c_func();

extern "C" void cpp_func()
{
	throw 1;
}

int main()
{
	try
	{
		c_func();
	}
	catch (int exception)
	{
		fprintf(stderr, "Exception correctly caught: %d\n", exception);
	}
}
// c_code.c
void cpp_func();

void c_func()
{
	cpp_func();
}

Let's build them like this:

clang++ ${FLAGS} -c cpp_code.cpp -o cpp_code.o
clang   ${FLAGS} -c c_code.c     -o c_code.o
clang++ ${FLAGS} c_code.o cpp_code.o -o test
./test

Now, everything depends on FLAGS. If we run this with empty FLAGS, the compiler will default to 64-bit mode, and the code will run correctly:

Exception correctly caught: 1

However, if we set FLAGS to -m32, forcing 32-bit mode, the result will be very different:

libc++abi.dylib: terminating with uncaught exception of type int

The problem observable here is undefined behavior of C++ exceptions passing C code barrier. It may run correctly if the ABI supports is (which is the case under 64-bit OSX), but it doesn't need to (like 32-bit OSX) - and then the code will crash as soon as the exception crosses C/C++ barrier.

I am running these tests on OSX (10.12.2) with Apple LLVM version 8.0.0 (clang-800.0.42.1), but the same behavior is visible under other versions of Clang and also under Linux.

@kjunichi

This comment has been minimized.

Show comment
Hide comment
@kjunichi

kjunichi Jan 6, 2017

Contributor

Hi, @dabroz .

clang   ${FLAGS} c_code.o cpp_code.o -o test

is it a typo?

clang++  ${FLAGS} c_code.o cpp_code.o -o test

is a collect,isn' it?

In this case, you alse can use -fexception option for compiling C code.

export FLAGS="-m32"
clang++ ${FLAGS} -c cpp_code.cpp -o cpp_code.o
clang   ${FLAGS} -fexceptions -c c_code.c     -o c_code.o
clang++  ${FLAGS} c_code.o cpp_code.o -o test
./test 
Exception correctly caught: 1

(I ran this on macOS.)

I'm not C/C++ expert, please see #2832 .

I think this is not related with murby.

Contributor

kjunichi commented Jan 6, 2017

Hi, @dabroz .

clang   ${FLAGS} c_code.o cpp_code.o -o test

is it a typo?

clang++  ${FLAGS} c_code.o cpp_code.o -o test

is a collect,isn' it?

In this case, you alse can use -fexception option for compiling C code.

export FLAGS="-m32"
clang++ ${FLAGS} -c cpp_code.cpp -o cpp_code.o
clang   ${FLAGS} -fexceptions -c c_code.c     -o c_code.o
clang++  ${FLAGS} c_code.o cpp_code.o -o test
./test 
Exception correctly caught: 1

(I ran this on macOS.)

I'm not C/C++ expert, please see #2832 .

I think this is not related with murby.

@dabroz

This comment has been minimized.

Show comment
Hide comment
@dabroz

dabroz Jan 6, 2017

Contributor

@kjunichi you're right, linking target binary also needs clang++. Fixed.

-fexceptions seems to work well on 32-bit Mac targets. However I would recommend to leave compatibility fixes (pointer casts etc.) in place, as being able to compile whole mruby as C++ can also be useful in some scenarios (embedding).

Contributor

dabroz commented Jan 6, 2017

@kjunichi you're right, linking target binary also needs clang++. Fixed.

-fexceptions seems to work well on 32-bit Mac targets. However I would recommend to leave compatibility fixes (pointer casts etc.) in place, as being able to compile whole mruby as C++ can also be useful in some scenarios (embedding).

@bggd

This comment has been minimized.

Show comment
Hide comment
@bggd

bggd Jan 7, 2017

Contributor

Can We add an option like enable_c_as_cxx or force_compile_as_cxx or something to mruby's Rake?

Contributor

bggd commented Jan 7, 2017

Can We add an option like enable_c_as_cxx or force_compile_as_cxx or something to mruby's Rake?

matz added a commit that referenced this pull request Feb 28, 2017

Compile C files by C compiler when C++ files mixed.
ref #3267 #3470

By this commit, mruby do not use C++ ABI mode unless
you specify explicitly. It compiles C files by C
compilers, with C++ exception enabled when it sees
C++ files in your configured mrbgems.

I haven't tried visualcpp, so please submit an issue
if you see any problem with C++ gems on Windows.
@matz

This comment has been minimized.

Show comment
Hide comment
@matz

matz Feb 28, 2017

Member

@dabroz could you check if the recent commit (1a1f834) works OK for you?

Member

matz commented Feb 28, 2017

@dabroz could you check if the recent commit (1a1f834) works OK for you?

@dabroz

This comment has been minimized.

Show comment
Hide comment
@dabroz

dabroz Mar 2, 2017

Contributor

@matz I'll check this update and see if tests can be updated to test all possible ABIs.

Contributor

dabroz commented Mar 2, 2017

@matz I'll check this update and see if tests can be updated to test all possible ABIs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment