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.

Sign up for GitHub

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

Jump to bottom

History output refactor broke nag #123

Closed
bandre-ucar opened this issue Sep 19, 2016 · 3 comments
Closed

History output refactor broke nag #123

bandre-ucar opened this issue Sep 19, 2016 · 3 comments

Comments

@bandre-ucar
Copy link
Contributor

Summary of Issue:

The merge of history output refactor, a6fec29, broke running the master branch with nag.

Expected behavior and actual behavior:

Expected behavior compile and run with nag.

Actual behavior, run time error:

Runtime Error: /home/andre/work/ed/pr110/hist-bug/components/clm/src/ED/main/HistoryIOMod.F90, line 1187: Reference to dangling pointer HVAR%IOVAR_DK_PTR%DIM1_PTR
Target was RETURNed from procedure HISTORYIOMOD:SET_DIM_PTRS
Program terminated by fatal error
/home/andre/work/ed/pr110/hist-bug/components/clm/src/ED/main/HistoryIOMod.F90, line 1187: Error occurred in HISTORYIOMOD:GET_HVAR_BOUNDS
/home/andre/work/ed/pr110/hist-bug/components/clm/src/ED/main/HistoryIOMod.F90, line 1126: Called by HISTORYIOMOD:SET_HISTORY_VAR
/home/andre/work/ed/pr110/hist-bug/components/clm/src/ED/main/HistoryIOMod.F90, line 755: Called by HISTORYIOMOD:DEFINE_HISTORY_VARS
/home/andre/work/ed/pr110/hist-bug/components/clm/src/utils/clmfates_interfaceMod.F90, line 1355: Called by CLMFATESINTERFACEMOD:INIT_HISTORY_IO
/home/andre/work/ed/pr110/hist-bug/components/clm/src/utils/clmfates_interfaceMod.F90, line 370: Called by CLMFATESINTERFACEMOD:INIT_ALLOCATE
/home/andre/work/ed/pr110/hist-bug/components/clm/src/main/clm_instMod.F90, line 408: Called by CLM_INSTMOD:CLM_INSTINIT
/home/andre/work/ed/pr110/hist-bug/components/clm/src/main/clm_initializeMod.F90, line 426: Called by CLM_INITIALIZEMOD:INITIALIZE2
/home/andre/work/ed/pr110/hist-bug/components/clm/src/cpl/lnd_comp_mct.F90, line 232: Called by LND_COMP_MCT:LND_INIT_MCT
/home/andre/work/ed/pr110/hist-bug/cime/driver_cpl/driver/component_mod.F90, line 233: Called by COMPONENT_MOD:COMPONENT_INIT_CC
/home/andre/work/ed/pr110/hist-bug/cime/driver_cpl/driver/cesm_comp_mod.F90, line 1161: Called by CESM_COMP_MOD:CESM_INIT
/home/andre/work/ed/pr110/hist-bug/cime/driver_cpl/driver/cesm_driver.F90, line 92: Called by CESM_DRIVER

Steps to reproduce the problem (should include create_newcase or create_test command along with any user_nl or xml changes):

cd cime/scripts
./create_test -testname SMS_D_Mmpi-serial_Ld5.5x5_amazon.ICLM45ED.hobart_nag.clm-edTest

What is the changeset ID of the code, and the machine you are using:

a6fec29

have you modified the code? If so, it must be committed and available for testing:

no

Screen output or output files showing the error message and context:

see above
@rgknox
Copy link
Contributor

rgknox commented Sep 20, 2016

ugg, will look into this

bandre-ucar added a commit that referenced this issue Sep 20, 2016
@bandre-ucar
Refactor to incorporate EDCLMLinkMod into the fates interface
1d86ab8 
Merge branch 'rgknox-CLMLink'

This changegroup is the first of two parts to convert the contents of
EDCLMLinkMod into the interface. Notable changes in this part:

1) SummarizeNetFluxes and ED_BGC_Carbon_Balancecheck are refactored to
use FATES memory structure

2) history output variables in SummarizeNetFluxes and
ED_BGC_Carbon_Balancecheck are migrated to the new scheme

3) restart variables in EDCLMLinkMod are migrated to EDRestVectorMod

Code review: rgknox

Testing:
  rgknox:
    Test suite: ed, clm_short_45, clm_short_50 - lawrencium-lr3 intel
                ed - yellowstone pgi, gnu, intel
    Test baseline: 0c8090b (rgknox-HIO)
    Test namelist changes: none
    Test answer changes: b4b

    Test summary: all PASS with expected fails for gnu compiler comparing
      clm history baselines, #122, fixed in #124:
        SMS_Ld5.f19_g16.ICLM45ED.yellowstone_gnu.clm-edTest
        SMS_Ld5.f10_f10.ICLM45ED.yellowstone_gnu.clm-edTest

  andre:
    Test suite: ed, clm_short - yellowstone gnu, intel, pgi
    Test baseline ed: 9f2b41c
    Test baseline clm_short: clm4_5_12_r193
    Test namelist changes: none
    Test answer changes: b4b
    Test summary: all tests pass

    Test suite: hobart-nag - not run, #123
@bandre-ucar
Copy link
Contributor Author

from Fortran 2008 (C.9.4, see also 12.5.2.4)

If a nonpointer dummy argument has the TARGET attribute and the corresponding actual argument does not, any pointers that become associated with the dummy argument, and therefore with the actual argument, during execution of the procedure, become undefined when execution of the procedure completes.

Fortran 2003, 4.5.3, R441, component attr spec of a derived type:

component-attr-spec is POINTER
or DIMENSION ( component-array-spec )
or ALLOCATABLE
or access-spec

Fortran 2003, 12.4.1.2

If the dummy argument has the TARGET attribute and the corresponding actual argument does not
have the TARGET attribute or is an array section with a vector subscript, any pointers associated with
the dummy argument become undefined when execution of the procedure completes.

So, my interpretation of the fortran standard is that: if a dummy arg to a procedure, e.g. set_dim_ptrs, dim_target, is a target, then the actual arg must be a target.

Declaring a something as a component of a derived type, a single component can not be a target, the instance of the entire derived type must be a target.

In the context of the history IO, tracing up through the function calls and nested derived types, the actual instance is the hlm_fates_interface_type, clm_fates, created in clm_instMod.F90.

Setting clm_fates to be a target in clm_instMod.F90 does not fix this problem. So either there is some other problem, or nag is getting confused....

The target attribute is largely used by the compiler to control (reduce?) optimization done on those objects. It's a bit uncomfortable that the entire clm_fates object must be a target just to have a few pointers to simple structs.

Given the general confusion and maintenance problems with pointers, I think it would be a better use of time to refactor the history io to use an object oriented approach with simple getters and setters instead of trying to straighten out the pointer issue.

@bandre-ucar bandre-ucar mentioned this issue Oct 6, 2016
Closed
@bandre-ucar bandre-ucar mentioned this issue Oct 26, 2016
Merged
@bandre-ucar
Copy link
Contributor Author

fixed and merged in 2ac7960

rgknox pushed a commit that referenced this issue Apr 21, 2017
@bandre-ucar
Refactor to incorporate EDCLMLinkMod into the fates interface
6e67127 
Merge branch 'rgknox-CLMLink'

This changegroup is the first of two parts to convert the contents of
EDCLMLinkMod into the interface. Notable changes in this part:

1) SummarizeNetFluxes and ED_BGC_Carbon_Balancecheck are refactored to
use FATES memory structure

2) history output variables in SummarizeNetFluxes and
ED_BGC_Carbon_Balancecheck are migrated to the new scheme

3) restart variables in EDCLMLinkMod are migrated to EDRestVectorMod

Code review: rgknox

Testing:
  rgknox:
    Test suite: ed, clm_short_45, clm_short_50 - lawrencium-lr3 intel
                ed - yellowstone pgi, gnu, intel
    Test baseline: 0c8090b (rgknox-HIO)
    Test namelist changes: none
    Test answer changes: b4b

    Test summary: all PASS with expected fails for gnu compiler comparing
      clm history baselines, #122, fixed in #124:
        SMS_Ld5.f19_g16.ICLM45ED.yellowstone_gnu.clm-edTest
        SMS_Ld5.f10_f10.ICLM45ED.yellowstone_gnu.clm-edTest

  andre:
    Test suite: ed, clm_short - yellowstone gnu, intel, pgi
    Test baseline ed: 9f2b41c
    Test baseline clm_short: clm4_5_12_r193
    Test namelist changes: none
    Test answer changes: b4b
    Test summary: all tests pass

    Test suite: hobart-nag - not run, #123
rgknox pushed a commit that referenced this issue May 23, 2017
@bandre-ucar
Refactor to incorporate EDCLMLinkMod into the fates interface
d35425c 
Merge branch 'rgknox-CLMLink'

This changegroup is the first of two parts to convert the contents of
EDCLMLinkMod into the interface. Notable changes in this part:

1) SummarizeNetFluxes and ED_BGC_Carbon_Balancecheck are refactored to
use FATES memory structure

2) history output variables in SummarizeNetFluxes and
ED_BGC_Carbon_Balancecheck are migrated to the new scheme

3) restart variables in EDCLMLinkMod are migrated to EDRestVectorMod

Code review: rgknox

Testing:
  rgknox:
    Test suite: ed, clm_short_45, clm_short_50 - lawrencium-lr3 intel
                ed - yellowstone pgi, gnu, intel
    Test baseline: 0c8090b (rgknox-HIO)
    Test namelist changes: none
    Test answer changes: b4b

    Test summary: all PASS with expected fails for gnu compiler comparing
      clm history baselines, #122, fixed in #124:
        SMS_Ld5.f19_g16.ICLM45ED.yellowstone_gnu.clm-edTest
        SMS_Ld5.f10_f10.ICLM45ED.yellowstone_gnu.clm-edTest

  andre:
    Test suite: ed, clm_short - yellowstone gnu, intel, pgi
    Test baseline ed: 9f2b41c
    Test baseline clm_short: clm4_5_12_r193
    Test namelist changes: none
    Test answer changes: b4b
    Test summary: all tests pass

    Test suite: hobart-nag - not run, #123
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@rgknox @bandre-ucar