Skip to content

fix two unitialized variables found w/ valgrind #662

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

Merged
billsacks merged 1 commit into from
Jun 8, 2026
Merged

fix two unitialized variables found w/ valgrind #662

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions mediator/esmFlds.F90
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,8 @@ module esmflds
!-----------------------------------------------

type, public :: med_fldList_entry_type
character(CS) :: stdname
character(CS) :: shortname
character(CS) :: stdname = ''
character(CS) :: shortname = ''
Comment on lines +48 to +49

@billsacks billsacks Jun 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm fine with this change, but it makes me wonder if this might be masking a more serious issue. I don't have experience with valgrind, so I'm not sure: does the issue picked up in #661 suggest an actual attempt to reference a field with previously-uninitialized values, or just that valgrind can't guarantee that the data was initialized? If the former, I'm wondering if the real solution is to figure out the code path that has led to the use of a field with uninitialized stdname / shortname? Otherwise, it seems like we might be masking the problem by satisfying valgrind but with non-sensical data. @DeniseWorthen @DusanJovic-NOAA what do you think?

@DeniseWorthen DeniseWorthen Jun 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

My initial fix was more complicated (partly a result of querying AI for assistance). Those code changes were

diff --git a/mediator/esmFlds.F90 b/mediator/esmFlds.F90
index 7a2959f4..18b832b0 100644
--- a/mediator/esmFlds.F90
+++ b/mediator/esmFlds.F90
@@ -188,11 +188,17 @@ contains
     lastfld => fields
     found = .false.
     do while(associated(lastfld%next))
-       if (trim(stdname) == trim(lastfld%stdname)) exit
+       if (trim(stdname) == trim(lastfld%stdname)) then
+          found = .true.
+          exit
+       end if
        lastfld => lastfld%next
     enddo
+
     ! Check the lastfld
+    if (.not. found) then
        if (trim(stdname) == trim(lastfld%stdname)) found = .true.
+    end if

The attempt here was to ensure that if a match was not found, lastfld%next would still point to the very last (found) field. But Dusan reported that it did not resolve the issue.

I also don't have valgrind experience, but Dusan provided instructions and I can try to work through a better fix if you have ideas.

@billsacks billsacks Jun 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

With Claude's help, I think I understand the problem: When med_fldList_findName is called on a currently-empty list, the last line (if (trim(stdname) == trim(lastfld%stdname)) found = .true.) will be working on uninitialized data.

Now that I understand that issue, the fix here makes sense to me.


! Mapping fldsFr data - for mediator import fields
integer , allocatable :: mapindex(:)
Expand Down
8 changes: 4 additions & 4 deletions mediator/med_merge_mod.F90
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ module med_merge_mod
use esmFlds , only : med_fldList_findName
use perf_mod , only : t_startf, t_stopf
use shr_log_mod , only : shr_log_error

implicit none
private

Expand Down Expand Up @@ -128,7 +128,7 @@ subroutine med_merge_auto_multi_fldbuns(coupling_active, FBOut, FBfrac, FBImp, f
call med_fld_GetFldInfo(fldptr, compsrc=compsrc, merge_fields=merge_fields, merge_type=merge_type, merge_fracname=merge_fracname, rc=rc)
if (chkerr(rc,__LINE__,u_FILE_u)) return

if (merge_type /= 'unset' .and. merge_field /= 'unset') then
if (merge_type /= 'unset' .and. merge_fields /= 'unset') then

@billsacks billsacks Jun 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree with this change - thanks.

! If merge_field is a colon delimited string then cycle through every field - otherwise by default nm
! will only equal 1
num_merge_colon_fields = merge_listGetNum(merge_fields)
Expand Down Expand Up @@ -489,7 +489,7 @@ subroutine med_merge_auto_errcheck(compsrc, fldname_out, field_out, &
write(errmsg,*) trim(subname),' input field ungriddedUbound ',ungriddedUbound_in(1),&
' for '//trim(merge_fldname), &
' not equal to output field ungriddedUbound ',ungriddedUbound_out,' for '//trim(fldname_out)

call shr_log_error(errmsg, rc=rc)
endif

Expand Down Expand Up @@ -647,7 +647,7 @@ subroutine med_merge_field_1D(FBout, fnameout, &
endif
if (wgtfound) then
if (lbound(dataPtr,1) /= lbound(wgt,1) .or. ubound(dataPtr,1) /= ubound(wgt,1)) then

call shr_log_error(trim(subname)//": ERROR wgt wrong size", &
line=__LINE__, file=u_FILE_u, rc=dbrc)
return
Expand Down
Loading