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

Convert IES to C++ #2312

Merged
merged 1 commit into from
Nov 8, 2021
Merged

Convert IES to C++ #2312

merged 1 commit into from
Nov 8, 2021

Conversation

dafeda
Copy link
Contributor

@dafeda dafeda commented Nov 4, 2021

Issue
Resolves #2310

Approach
I wrapped all function in ies_enkf.cpp in extern C to make dlsym work.
Is there a better way?

Other than that, I had to do a few ´static_casts`.

@dafeda dafeda requested a review from pinkwah November 4, 2021 08:59
@dafeda dafeda force-pushed the ies_enkf_cpp branch 2 times, most recently from 22c1440 to 28ac8e4 Compare November 4, 2021 09:23
@pinkwah
Copy link
Contributor

pinkwah commented Nov 4, 2021

Did you get an error when you didn't have extern "C"? Iirc, we use dlsym to find the EXTERNAL_MODULE_SYMBOL symbol, not any of the functions.

@dafeda
Copy link
Contributor Author

dafeda commented Nov 4, 2021

Did you get an error when you didn't have extern "C"? Iirc, we use dlsym to find the EXTERNAL_MODULE_SYMBOL symbol, not any of the functions.

Here's the error I get without extern C:

Consolidate compiler generated dependencies of target ies_std_compare
[ 63%] Building CXX object lib/CMakeFiles/ies_std_compare.dir/analysis/modules/tests/ies_std_compare.cpp.o
[ 64%] Linking CXX executable ies_std_compare
Undefined symbols for architecture x86_64:
  "_ies_enkf_init_update", referenced from:
      cmp_std_ies(res::es_testdata const&) in ies_std_compare.cpp.o
  "_ies_enkf_updateA", referenced from:
      cmp_std_ies(res::es_testdata const&) in ies_std_compare.cpp.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[2]: *** [lib/ies_std_compare] Error 1
make[1]: *** [lib/CMakeFiles/ies_std_compare.dir/all] Error 2
make: *** [all] Error 2

My understanding is that we load ies using dlsym but this won't work if the functions are not wrapped in extern C because of C++'s name-mangling.

@pinkwah
Copy link
Contributor

pinkwah commented Nov 4, 2021

You wouldn't be getting linking errors if this was a dlsym, as that is a dynamic lookup at runtime.

This particular error occurs because the function is declared with extern "C" in the header file:

#ifdef __cplusplus
extern "C" {
#endif
void ies_enkf_init_update(void *arg, const bool_vector_type *ens_mask,
const bool_vector_type *obs_mask,
const matrix_type *S, const matrix_type *R,
const matrix_type *dObs, const matrix_type *E,
const matrix_type *D, rng_type *rng);

@pinkwah
Copy link
Contributor

pinkwah commented Nov 4, 2021

Otherwise this PR looks good.

@dafeda
Copy link
Contributor Author

dafeda commented Nov 4, 2021

You wouldn't be getting linking errors if this was a dlsym, as that is a dynamic lookup at runtime.

This particular error occurs because the function is declared with extern "C" in the header file:

#ifdef __cplusplus
extern "C" {
#endif
void ies_enkf_init_update(void *arg, const bool_vector_type *ens_mask,
const bool_vector_type *obs_mask,
const matrix_type *S, const matrix_type *R,
const matrix_type *dObs, const matrix_type *E,
const matrix_type *D, rng_type *rng);

I tried removing extern C from ies_enkf.hpp and reverted the change in ies_enkf.cpp, see last commit.
I'm now getting the same error as before.
It's still a linker error where the test ies_std_compare.cpp can't find symbols.

@dafeda dafeda self-assigned this Nov 5, 2021
@dafeda dafeda force-pushed the ies_enkf_cpp branch 2 times, most recently from eee6d57 to 8d7ffed Compare November 5, 2021 10:44
matrix_type *dObs, // Actual observations (not used)
matrix_type *Ein, // Ensemble of observation perturbations
matrix_type *Din, // (d+E-Y) Ensemble of perturbed observations - Y
matrix_type *A, // Updated ensemble A retured to ERT.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo in comment

.freef = ies_enkf_data_free,
.has_var = ies_enkf_has_var,
.alloc = ies_enkf_data_alloc,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these changing place?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the C++ compiler complains that they are not in the same order as defined in analysis_table.hpp.
C-compiler did not care.

@dafeda dafeda merged commit 208bcd9 into equinor:main Nov 8, 2021
@dafeda dafeda deleted the ies_enkf_cpp branch November 8, 2021 12:18
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

Successfully merging this pull request may close these issues.

Convert ies_enkf to C++
3 participants