fix two unitialized variables found w/ valgrind#662
Conversation
billsacks
left a comment
There was a problem hiding this comment.
Thanks a lot for these changes. I'm interested in hearing your thoughts on my comment below.
| character(CS) :: stdname = '' | ||
| character(CS) :: shortname = '' |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
I agree with this change - thanks.
Description of changes
Initialize two character strings and correct typo.
Contributors other than yourself, if any: @DusanJovic-NOAA
Changes are B4B in UFS testing.