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 pinning cxx_version in get_stdopt for xeus-cpp-lite #202

Closed
anutosh491 opened this issue Jan 8, 2025 · 2 comments · Fixed by #203
Closed

Remove pinning cxx_version in get_stdopt for xeus-cpp-lite #202

anutosh491 opened this issue Jan 8, 2025 · 2 comments · Fixed by #203
Labels
bug Something isn't working

Comments

@anutosh491
Copy link
Collaborator

anutosh491 commented Jan 8, 2025

Currently we only support c++ 20 based wasm kernel and this flag is passed to our wasm interpreter here

void* createInterpreter(const Args &ExtraArgs = {}) {
Args ClangArgs = {/*"-xc++"*/"-v"}; // ? {"-Xclang", "-emit-llvm-only", "-Xclang", "-diagnostic-log-file", "-Xclang", "-", "-xc++"};
#ifdef EMSCRIPTEN
ClangArgs.push_back("-std=c++20");
#else

This is the only place where essentially we should be hardcoing the version 20 before we start supporting multiple wasm kernels.

Currently get_stdopt is framed like this

    static std::string get_stdopt()
    {
        // We need to find what's the C++ version the interpreter runs with.
#ifndef EMSCRIPTEN
        const char* code = R"(
int __get_cxx_version () {
#if __cplusplus > 202302L
    return 26;
#elif __cplusplus > 202002L
    return 23;
#elif __cplusplus >  201703L
    return 20;
#elif __cplusplus > 201402L
    return 17;
#elif __cplusplus > 201103L || (defined(_WIN32) && _MSC_VER >= 1900)
    return 14;
#elif __cplusplus >= 201103L
   return 11;
#else
  return 0;
#endif
  }
__get_cxx_version ()
      )";
        auto cxx_version = Cpp::Evaluate(code);
        return std::to_string(cxx_version);
#else
        return "20";
#endif
    }
    

But we should simply have

    static std::string get_stdopt()
    {
        // We need to find what's the C++ version the interpreter runs with.
        const char* code = R"(
int __get_cxx_version () {
#if __cplusplus > 202302L
    return 26;
#elif __cplusplus > 202002L
    return 23;
#elif __cplusplus >  201703L
    return 20;
#elif __cplusplus > 201402L
    return 17;
#elif __cplusplus > 201103L || (defined(_WIN32) && _MSC_VER >= 1900)
    return 14;
#elif __cplusplus >= 201103L
   return 11;
#else
  return 0;
#endif
  }
__get_cxx_version()
      )";
        auto cxx_version = Cpp::Evaluate(code);
        return std::to_string(cxx_version);
    }

That is basically run the __get_cxx_version as a first side module and fetch the version through Evaluate rather than hardcode it, cause xeus-cpp-lite should be able to run the code from get_stdopt anyways.

@github-actions github-actions bot added the Needs triage Used in auto labelling of new issues label Jan 8, 2025
@anutosh491
Copy link
Collaborator Author

anutosh491 commented Jan 8, 2025

Currently if we do it, we end up with

image

Which is expected.

@anutosh491
Copy link
Collaborator Author

This happens because of how __clang_Interpreter_SetValueNoAlloc is defined (https://github.com/llvm/llvm-project/blob/5656cbca52545e608f6fb8b7c9a778c7c9b4b468/clang/lib/Interpreter/InterpreterValuePrinter.cpp#L313)

So the solution is as follows.

  1. First expose the symbol through libclangCppInterOp.so

Probably adding the following below this block(https://github.com/compiler-research/CppInterOp/blob/c446c3e2c43a1d3f374c739295d1484d5349274e/lib/Interpreter/CMakeLists.txt#L7-L19) would suffice.

  target_link_options(clangCppInterOp PRIVATE
    "-Wl,--export=__clang_Interpreter_SetValueNoAlloc"
  )
  1. Check if the symbol is being exported by libclangCppInterop.so. We might expect something like this
wasm-objdump -x libclangCppInterOp.so | grep __clang_Interpreter_SetValueNoAlloc

- func[854] sig=6 <__clang_Interpreter_SetValueNoAlloc>
 - func[854] <__clang_Interpreter_SetValueNoAlloc> -> "__clang_Interpreter_SetValueNoAlloc"
 - func[854] size=3144 <__clang_Interpreter_SetValueNoAlloc>
  1. Further it should be exposed in xcpp.wasm (through libclangCppInterOp.so ... this might happen itself without intervention) so that the first side module can make use of it. Probably should see the following in xcpp.wasm
(func $__clang_Interpreter_SetValueNoAlloc (;480;) (export "__clang_Interpreter_SetValueNoAlloc") (param $var0 i32) (param $var1 i32) (param $var2 i32) (param $var3 i32)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant