- edited description
segfault in ID_converter_ILGRMHD when Avec and Aphi are not given storage outside of ILGRMHD
git hash e46f4f5 "Major update to IllinoisGRMHD/Convert_to_HydroBase/ID_converter_ILGRMHD: Improve efficiency (e.g., remove unnecessary additional Newton-Raphson iterations in con2prim), remove u0 gridfunction, bugfix, separate IllinoisGRMHD::grmhd_primitives_Bi and IllinoisGRMHD::grmhd_primitives_Bi_stagger" of wvuthorns introduced these lines:
if(pure_hydro_run) {
#pragma omp parallel for
for(int k=0;k<cctk_lsh[2];k++) for(int j=0;j<cctk_lsh[1];j++) for(int i=0;i<cctk_lsh[0];i++) {
int index=CCTK_GFINDEX3D(cctkGH,i,j,k);
Avec[CCTK_GFINDEX4D(cctkGH,i,j,k,0)]=0;
Avec[CCTK_GFINDEX4D(cctkGH,i,j,k,1)]=0;
Avec[CCTK_GFINDEX4D(cctkGH,i,j,k,2)]=0;
Aphi[index]=0;
}
}
which cause asegfault when AVec or Aphi have no storage. ID_converter_ILGRMHD does not take measures to ensure that there actually be storage so eg the example parfile from http://astro.phys.wvu.edu/zetienne/ILGRMHD/getstarted.html segfaults.
ID_converter_ILGRMHD (and presumably) ILGRMHD itself must either ensure that there is storage for variables they unconditionally access or contain logic to avoid accessing variables that have no storage.
Comments (7)
-
reporter -
reporter - edited description
-
This can be fixed with just a few-line patch, right?
Here’s a proposed patch:
diff --git a/ID_converter_ILGRMHD/schedule.ccl b/ID_converter_ILGRMHD/schedule.ccl index 626c9a6..57f5c1a 100644 --- a/ID_converter_ILGRMHD/schedule.ccl +++ b/ID_converter_ILGRMHD/schedule.ccl @@ -1,5 +1,9 @@ # Schedule definitions for thorn ID_converter_ILGRMHD +if (!pure_hydro_run) { + STORAGE: HydroBase::Avec[1] HydroBase::Aphi[1] +} + schedule group IllinoisGRMHD_ID_Converter at CCTK_INITIAL after HydroBase_Initial before Convert_to_HydroBase { } "Translate ET-generated, HydroBase-compatible initial data and convert into variables used by IllinoisGRMHD"
-
reporter That may fix it, yes. Are there any other variables that are similarly used and do not always have storage? ADMBase variables and maybe Bvec in HydroBase?
-
A single timelevel of storage is allocated for Bvec in Convert_to_HydroBase & GiRaFFE_to_HydroBase, which are the only thorns that write to it.
Regarding ADMBase variables, they must first be set in CCTK_INITIAL outside IllinoisGRMHD, otherwise it doesn’t make sense to be using ID_converter_* (which by its nature should be able to assume that ID already exist!) The story is the same for vel[], rho, press, and eps gridfunctions – these must be set within CCTK_INITIAL or it doesn’t make sense to be calling on an ID converter thorn.
There was a similar issue in ID_converter_GiRaFFE, in which there was the same parameter (lazily copied from ILGRMHD) “pure_hydro_run”, but this is nonsense in the case of GRFFE (which assumes a magnetically dominated plasma), so I removed it.
First attempt at a fix:
https://bitbucket.org/zach_etienne/wvuthorns/commits/6313730fe1a0588fcaeddfe1552f776ab9c7929a
Second time’s a charm:
https://bitbucket.org/zach_etienne/wvuthorns/commits/15645db5265c2a7e2415d5073c72c2112b002a33
-
reporter -
reporter - changed status to resolved
I would normally suggest this to be backported to the release branch (as an important fix). However given that the next release is in 11 days this may not be needed.
- Log in to comment