[
https://issues.apache.org/jira/browse/IMPALA-11412?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Daniel Becker updated IMPALA-11412:
-----------------------------------
Description:
The function template {{CodegenFnPtr<FuncType>::store()}} tries to implicitly
cast a function pointer (of type {{FuncType}}) to {{void*}}, which is a compile
time error. The reason this didn't come up in the builds is that this function
template is currently not used anywhere, and the function pointers are stored
through the parent class, using {{{}CodegenFnPtrBase::store(){}}}, which takes
a {{{}void*{}}}.
We should either
# remove the hitherto unused {{CodegenFnPtr<FuncType>::store()}} function
template
OR
# add the correct explicit cast from function pointer to {{void*}} AND add a
test which instantiates (and tests) this function template so we can be sure
that the new implementation is correct.
I'm inclined to choose the second option because I think the interface of
{{CodegenFnPtr<FuncType>}} is more complete if we have this function as well,
even if it is currently not used.
Note:
After digging a bit on the internet I found that the reason that implicit
function pointer to {{void*}} cast is not allowed (as opposed to implicit
regular pointer to {{void*}}) is because the standard doesn't guarantee that
regular and function pointers have the same size, and there are some
architectures where they actually don't.
However, according to 8) on
[https://en.cppreference.com/w/cpp/language/reinterpret_cast], POSIX compliant
systems do have this guarantee, so it shouldn't be a problem that we store
function pointers as {{{}void*{}}}. We don't really have a choice because LLVM
does the same as {{llvm::ExecutionEngine::getPointerToFunction()}} returns a
{{void*}} (see
[https://llvm.org/doxygen/classllvm_1_1ExecutionEngine.html#acc46759a6acfc3d116c3f22110326ffa]);
we call that function
[here|https://github.com/apache/impala/blob/fefb9f24be1f99ac0077a8d6ef00834d8e90ef45/be/src/codegen/llvm-codegen.cc#L1315].
was:
The function template {{CodegenFnPtr<FuncType>::store()}} tries to implicitly
cast a function pointer (of type {{FuncType}}) to {{void*}}, which is a compile
time error. The reason this didn't come up in the builds is that this function
template is currently not used anywhere, and the function pointers are stored
through the parent class, using {{{}CodegenFnPtrBase::store(){}}}, which takes
a {{{}void*{}}}.
We should either
# remove the hitherto unused {{CodegenFnPtr<FuncType>::store()}} function
template
OR
# add the correct explicit cast from function pointer to {{void*}} AND add a
test which instantiates (and tests) this function template so we can be sure
that the new implementation is correct.
I'm inclined to choose the second option because I think the interface of
{{CodegenFnPtr<FuncType>}} is more complete if we have this function as well,
even if it is currently not used.
Note:
After digging a bit on the internet I found that the reason that implicit
function pointer to {{{{void*}}}} cast is not allowed (as opposed to implicit
regular pointer to {{{}void*{}}}) is because the standard doesn't guarantee
that regular and function pointers have the same size, and there are some
architectures where they actually don't.
However, according to 8) on
[https://en.cppreference.com/w/cpp/language/reinterpret_cast], POSIX compliant
systems do have this guarantee, so it shouldn't be a problem that we store
function pointers as {{{}void*{}}}. We don't really have a choice because LLVM
does the same as {{llvm::ExecutionEngine::getPointerToFunction()}} returns a
{{void*}} (see
[https://llvm.org/doxygen/classllvm_1_1ExecutionEngine.html#acc46759a6acfc3d116c3f22110326ffa]);
we call that function
[here|https://github.com/apache/impala/blob/fefb9f24be1f99ac0077a8d6ef00834d8e90ef45/be/src/codegen/llvm-codegen.cc#L1315].
> CodegenFnPtr<FuncType>::store() has a compile time error when instantiated
> --------------------------------------------------------------------------
>
> Key: IMPALA-11412
> URL: https://issues.apache.org/jira/browse/IMPALA-11412
> Project: IMPALA
> Issue Type: Bug
> Components: Backend
> Reporter: Daniel Becker
> Assignee: Daniel Becker
> Priority: Major
>
> The function template {{CodegenFnPtr<FuncType>::store()}} tries to implicitly
> cast a function pointer (of type {{FuncType}}) to {{void*}}, which is a
> compile time error. The reason this didn't come up in the builds is that this
> function template is currently not used anywhere, and the function pointers
> are stored through the parent class, using {{{}CodegenFnPtrBase::store(){}}},
> which takes a {{{}void*{}}}.
> We should either
> # remove the hitherto unused {{CodegenFnPtr<FuncType>::store()}} function
> template
> OR
> # add the correct explicit cast from function pointer to {{void*}} AND add a
> test which instantiates (and tests) this function template so we can be sure
> that the new implementation is correct.
> I'm inclined to choose the second option because I think the interface of
> {{CodegenFnPtr<FuncType>}} is more complete if we have this function as well,
> even if it is currently not used.
> Note:
> After digging a bit on the internet I found that the reason that implicit
> function pointer to {{void*}} cast is not allowed (as opposed to implicit
> regular pointer to {{void*}}) is because the standard doesn't guarantee that
> regular and function pointers have the same size, and there are some
> architectures where they actually don't.
> However, according to 8) on
> [https://en.cppreference.com/w/cpp/language/reinterpret_cast], POSIX
> compliant systems do have this guarantee, so it shouldn't be a problem that
> we store function pointers as {{{}void*{}}}. We don't really have a choice
> because LLVM does the same as
> {{llvm::ExecutionEngine::getPointerToFunction()}} returns a {{void*}} (see
> [https://llvm.org/doxygen/classllvm_1_1ExecutionEngine.html#acc46759a6acfc3d116c3f22110326ffa]);
> we call that function
> [here|https://github.com/apache/impala/blob/fefb9f24be1f99ac0077a8d6ef00834d8e90ef45/be/src/codegen/llvm-codegen.cc#L1315].
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-all-unsubscr...@impala.apache.org
For additional commands, e-mail: issues-all-h...@impala.apache.org
