niyue commented on code in PR #37752: URL: https://github.com/apache/arrow/pull/37752#discussion_r1328095926
########## cpp/src/gandiva/engine.cc: ########## @@ -88,6 +89,7 @@ std::once_flag llvm_init_once_flag; static bool llvm_init = false; static llvm::StringRef cpu_name; static llvm::SmallVector<std::string, 10> cpu_attrs; +std::once_flag register_exported_funcs_flag; Review Comment: > If you use the REGISTER_EXPORTED_FUNCS macro in exported_funcs.cc Do you mean using the `REGISTER_EXPORTED_FUNCS` macro in `exported_funcs.cc` without putting them in a function? That is what I did initially, but I found one of the test case in CI `gandiva-projector-test-static` failed with a segfault (for example, this job https://github.com/apache/arrow/actions/runs/6210920546/job/16859620873?pr=37752). With the initial approach, I could pass all the tests locally (macOS 13, Apple M1 Max) but there is only `gandiva-projector-test` but no such `gandiva-projector-test-static` test when running locally (I probably have to look into what CI did to understand the difference). After looking at the code, I guess it may be caused by the C++ static initialization order issue (https://pabloariasal.github.io/2020/01/02/static-variable-initialization/#the-red-zone---static-initialization-order-fiasco). The `REGISTER_EXPORTED_FUNCS` macro used to register all the functions in `exported_funcs.cc` directly are not guaranteed to happen before their usage (registration and their usage are in different compilation units). That's why I made another change to put all the registration inside a function, and call this function explicitly once to guarantee the registration happens before its usage. Please let me know if there is any better way to address this issue. Thanks. ########## cpp/src/gandiva/engine.cc: ########## @@ -88,6 +89,7 @@ std::once_flag llvm_init_once_flag; static bool llvm_init = false; static llvm::StringRef cpu_name; static llvm::SmallVector<std::string, 10> cpu_attrs; +std::once_flag register_exported_funcs_flag; Review Comment: > If you use the REGISTER_EXPORTED_FUNCS macro in exported_funcs.cc Do you mean using the `REGISTER_EXPORTED_FUNCS` macro in `exported_funcs.cc` without putting them in a function? That is what I did initially, but I found one of the test case in CI `gandiva-projector-test-static` failed with a segfault (for example, this job https://github.com/apache/arrow/actions/runs/6210920546/job/16859620873?pr=37752). With the initial approach, I could pass all the tests locally (macOS 13, Apple M1 Max) but there is only `gandiva-projector-test` but no such `gandiva-projector-test-static` test when running locally (I probably have to look into what CI did to understand the difference). After looking at the code, I guess it may be caused by the C++ static initialization order issue (https://pabloariasal.github.io/2020/01/02/static-variable-initialization/#the-red-zone---static-initialization-order-fiasco). The `REGISTER_EXPORTED_FUNCS` macro used to register all the functions in `exported_funcs.cc` directly are not guaranteed to happen before their usage (registration and their usage are in different compilation units). That's why I made another change to put all the registration inside a function, and call this function explicitly once to guarantee the registration happens before its usage. Please let me know if there is any better way to address this issue. Thanks. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
