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

C++ virtual functions crash when called from global constructor #17295

Open
Maksim-Bozhko opened this issue Jun 22, 2022 · 5 comments
Open

C++ virtual functions crash when called from global constructor #17295

Maksim-Bozhko opened this issue Jun 22, 2022 · 5 comments

Comments

@Maksim-Bozhko
Copy link

Hello, we have a header with public interface that is exported and private implementation located in shared library(built with SIDE_MODULE=1). In constructor of global object we get reference to public interface and try to call virtual function, which leads to crash. We had similar problem before mentioned here: #17150
It was fixed, but now similar issue happens when calling virtual functions in constructors of global objects.

Version of emscripten/emsdk:
emcc (Emscripten gcc/clang-like replacement + linker emulating GNU ld) 3.1.14 (4343cbe)
clang version 15.0.0 (https://github.com/llvm/llvm-project 7effcbda49ba32991b8955821b8fdbd4f8f303e2)
Target: wasm32-unknown-emscripten
Thread model: posix

//Interface.h
#pragma once
#include <iostream>

class __attribute__((visibility ("default"))) PublicInterface
{
public:
	virtual void Foo() = 0;
	static PublicInterface& GetInstance();
};

class SomeClass
{
public:
	SomeClass() {
		std::cout << "SomeClass()" << "\n";
		PublicInterface::GetInstance().Foo();
	}
};

//Implementation.cpp
#include "Interface.h"

class PrivateImplementation : public PublicInterface
{
public:
	void Foo() override {
		std::cout << "PrivateImplementation::Foo()" << "\n";
	}
};
  
PublicInterface& PublicInterface::GetInstance() {
	std::cout << "PublicInterface::GetInstance()" << "\n";
	static PrivateImplementation privateImpl;
	return privateImpl;
}

//main.cpp
#include "Interface.h"

SomeClass g_var;
  
int main() {
	return 0;
}

Build commands
emcc Implementation.cpp -fPIC -fvisibility=hidden -c -o Implementation.o
emcc Implementation.o -sSIDE_MODULE=1 -o dylib1.wasm
emcc main.cpp -fPIC -fvisibility=hidden -c -o main.o
emcc main.o -sMAIN_MODULE=1 -o main.html dylib1.wasm

Console output
SomeClass()
PublicInterface::GetInstance()
Uncaught RuntimeError: null function or function signature mismatch

If I reverse runtime callback order in javascript so that global constructors of shared lib run before global constructors of main, problem dissappears, like this

function callRuntimeCallbacks(callbacks) {
callbacks.reverse(); // added this line
while (callbacks.length > 0) {
var callback = callbacks.shift();
if (typeof callback == 'function')
@Maksim-Bozhko
Copy link
Author

Maksim-Bozhko commented Jun 23, 2022

@sbc100 Hi, if I revert this #13668, my example starts working. Not everything is working properly in other casese, so complete fix is porbably more complicated than just revert.

@sbc100
Copy link
Collaborator

sbc100 commented Jun 23, 2022

Thanks for the report. It occurs to me that you could equally well create a case with the rolls of the side module and the main module reversed so we most likely need a more radical change here. It seems to me that we need apply relocations to all modules before we can run any user code (any global ctors)... to simply performing relocation as part of __wasm_call_ctors cannot work in the general case.

I think we will need to export a separate __wasm_apply_relocs which we can call on all the modules before call any constructors.

@Maksim-Bozhko
Copy link
Author

Maksim-Bozhko commented Jun 23, 2022

Thank you, separating __wasm_apply_relocs could be also helpful to reduce size and get around function size limit, I think this function could get pretty huge in some circumstances and even hit function size limit. Would it help if we enforce order of global constructors for modules, so that it matches order in which modules load? The reason it works in pure C++ is because our dependency shared lib that contains private code is loaded earlier and it's global constructors run earlier than callers globals. I am assuming vtable of class is not constucted yet in our case?

@sbc100
Copy link
Collaborator

sbc100 commented Jun 23, 2022

Sadly calling __wasm_apply_relocs directly won't change the size of that function .. which is the problematic one.

sbc100 added a commit that referenced this issue Jun 24, 2022
Until now we have applied data reloctions at the beginning of
`__wasm_call_ctors`.  However, in #17295, it was observed that this
is not valid, since relocation need to be applied to all loaded
libraries before any static constructor can be run.

To deal with this issue we now export `__wasm_apply_data_relocs`
separately and call this function on all libraries before calling any
`__wasm_call_ctors` function.

This change must land before the corresponding llvm-side change:
https://reviews.llvm.org/D128515.
sbc100 added a commit that referenced this issue Jun 24, 2022
Until now we have applied data reloctions at the beginning of
`__wasm_call_ctors`.  However, in #17295, it was observed that this
is not valid, since relocation need to be applied to all loaded
libraries before any static constructor can be run.

To deal with this issue we now export `__wasm_apply_data_relocs`
separately and call this function on all libraries before calling any
`__wasm_call_ctors` function.

This change must land before the corresponding llvm-side change:
https://reviews.llvm.org/D128515.  The tests for this change are
temporarily disabled until the llvm change gets rolled in.
sbc100 added a commit that referenced this issue Jun 24, 2022
Until now we have applied data reloctions at the beginning of
`__wasm_call_ctors`.  However, in #17295, it was observed that this
is not valid, since relocation need to be applied to all loaded
libraries before any static constructor can be run.

To deal with this issue we now export `__wasm_apply_data_relocs`
separately and call this function on all libraries before calling any
`__wasm_call_ctors` function.

This change must land before the corresponding llvm-side change:
https://reviews.llvm.org/D128515.  The tests for this change are
temporarily disabled until the llvm change gets rolled in.
sbc100 added a commit that referenced this issue Jun 24, 2022
Until now we have applied data reloctions at the beginning of
`__wasm_call_ctors`.  However, in #17295, it was observed that this
is not valid, since relocation need to be applied to all loaded
libraries before any static constructor can be run.

To deal with this issue we now export `__wasm_apply_data_relocs`
separately and call this function on all libraries before calling any
`__wasm_call_ctors` function.

This change must land before the corresponding llvm-side change:
https://reviews.llvm.org/D128515.  The tests for this change are
temporarily disabled until the llvm change gets rolled in.
sbc100 added a commit that referenced this issue Jun 24, 2022
Until now we have applied data reloctions at the beginning of
`__wasm_call_ctors`.  However, in #17295, it was observed that this
is not valid, since relocation need to be applied to all loaded
libraries before any static constructor can be run.

To deal with this issue we now export `__wasm_apply_data_relocs`
separately and call this function on all libraries before calling any
`__wasm_call_ctors` function.

This change must land before the corresponding llvm-side change:
https://reviews.llvm.org/D128515.  The tests for this change are
temporarily disabled until the llvm change gets rolled in.
sbc100 added a commit that referenced this issue Jun 24, 2022
Until now we have applied data reloctions at the beginning of
`__wasm_call_ctors`.  However, in #17295, it was observed that this
is not valid, since relocation need to be applied to all loaded
libraries before any static constructor can be run.

To deal with this issue we now export `__wasm_apply_data_relocs`
separately and call this function on all libraries before calling any
`__wasm_call_ctors` function.

This change must land before the corresponding llvm-side change:
https://reviews.llvm.org/D128515.  The tests for this change are
temporarily disabled until the llvm change gets rolled in.
sbc100 added a commit to llvm/llvm-project that referenced this issue Jun 27, 2022
… time

Instead, export `__wasm_apply_data_relocs` and `__wasm_call_ctors`
separately.

This is required since user code in a shared library (such as static
constructors) should not be run until relocations have been applied to
all loaded libraries.

See: emscripten-core/emscripten#17295

Differential Revision: https://reviews.llvm.org/D128515
@Maksim-Bozhko
Copy link
Author

Maksim-Bozhko commented Jun 29, 2022

Thanks for quick response and fixes!

mem-frob pushed a commit to draperlaboratory/hope-llvm-project that referenced this issue Oct 7, 2022
… time

Instead, export `__wasm_apply_data_relocs` and `__wasm_call_ctors`
separately.

This is required since user code in a shared library (such as static
constructors) should not be run until relocations have been applied to
all loaded libraries.

See: emscripten-core/emscripten#17295

Differential Revision: https://reviews.llvm.org/D128515
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

No branches or pull requests

2 participants