Skip to content

Fix Valgrind memory errors#1263

Merged
feldergast merged 1 commit intosstsimulator:develfrom
leekillough:valgrind_errors
Apr 3, 2025
Merged

Fix Valgrind memory errors#1263
feldergast merged 1 commit intosstsimulator:develfrom
leekillough:valgrind_errors

Conversation

@leekillough
Copy link
Contributor

@leekillough leekillough commented Mar 17, 2025

This fixes some Valgrind memory errors:

  1. In ConfigComponent, slot_num and statLoadLevel were not being initialized (maybe because they only apply in some cases -- a slot_num comment says "Only valid for subcomponents") and when ConfigComponent is serialized, it causes Valgrind to report an uninitialized memory read. This change simply adds slot_num(0) and statLoadLevel(0) to the constructors to initialize them to 0 and avoid uninitialized memory reads.
  2. In the SSTPythonModelDefinition constructor, an argv array is malloc()ed and then each of the argument pointers are separately malloc()ed. But then when it finishes, it only free()s the argv array, but not its individual arguments.
  3. In pymodel_comp.cc, compInit() allocates a string by calling gModel->addNamePrefix(name) and then passes it as a char* to gModel->addComponent(), which constructs a std::string copy of it by implicitly converting the char* argument to std::string. But then it is not freed. It would have been better if addNamePrefix() returned a std::string which automatically gets destroyed, but it would be too much work to change and test changing this API, so the pointer returned by addComponent() is free()ed after it is converted to a std::string in the call to addComponent().
  4. In CoreTestCheckPoint, random number generators are initialized with new during construction, but are not deleted in the destructor. As a precaution, all pointers to allocated memory are default-initialized to nullptr before the constructor body begins, so that they can be passed to delete even if they never got initialized with new (say, if an exception was thrown in the constructor). There is another memory leak in CoreTestCheckPoint::clock_handler, but because of how it is handled during checkpoint/restart, it is not as simple as delete-ing it in the destructor, since it is re-initialized strangely during checkpoint/restart and the CoreTestCheckPoint is not completely destroyed and re-constructed during checkpoint/restart phases, but it is left in a partial state and re-initialized. So this leak, which is reported as a "definite leak" by Valgrind, will require a better solution. Since this CoreTestCheckPoint does not appear to be a part of main Core but rather testing, and the leaks were found with Valgrind while running sst-test-core, it is lower priority that these leaks be fixed.

As a bonus, but not fixing any memory errors, ComponentInfo already had an explicitly-defined move constructor, but it did not use std::move() on two of its std::string and std::vector members, so std::move() was added so that the std::string and std::vector have shallow moves instead of deep copies.

Also Simulation_impl::getComponentInfoMap(), which returns the Component Info Map, was marked const, since it is best practice that getters should be const.

@github-actions github-actions bot added AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) AT: CLANG-FORMAT PASS and removed AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) labels Mar 17, 2025
@github-actions
Copy link

github-actions bot commented Mar 17, 2025

CLANG-FORMAT TEST - PASSED

@github-actions github-actions bot added AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) AT: CMAKE-FORMAT PASS and removed AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) labels Mar 17, 2025
@github-actions
Copy link

github-actions bot commented Mar 17, 2025

CMAKE-FORMAT TEST - PASSED

@sst-autotester
Copy link
Contributor

sst-autotester commented Mar 17, 2025

Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging
NO INSPECTION HAS BEEN PERFORMED ON THIS PULL REQUEST! - This PR must be inspected by setting label 'AT: PRE-TEST INSPECTED'.

@feldergast feldergast added the AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) label Mar 18, 2025
@github-actions github-actions bot removed the AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) label Mar 23, 2025
@github-actions
Copy link

github-actions bot commented Mar 23, 2025

CLANG-FORMAT TEST - PASSED

@github-actions github-actions bot added AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) and removed AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) labels Mar 23, 2025
@github-actions
Copy link

github-actions bot commented Mar 23, 2025

CMAKE-FORMAT TEST - PASSED

@sst-autotester
Copy link
Contributor

sst-autotester commented Mar 23, 2025

Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging
NO INSPECTION HAS BEEN PERFORMED ON THIS PULL REQUEST! - This PR must be inspected by setting label 'AT: PRE-TEST INSPECTED'.

@feldergast feldergast added the AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) label Mar 23, 2025
@leekillough
Fix memory leaks and uninitialized memory reads reported by Valgrind
9441945 
Improve ComponentInfo move constructor

Make getComponentInfoMap() const
@github-actions github-actions bot removed the AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) label Apr 1, 2025
@github-actions
Copy link

github-actions bot commented Apr 1, 2025

CLANG-FORMAT TEST - PASSED

@github-actions github-actions bot added AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) and removed AT: WIP Mark PR as a Work in Progress (No Autotesting Performed) labels Apr 1, 2025
@github-actions
Copy link

github-actions bot commented Apr 1, 2025

CMAKE-FORMAT TEST - PASSED

@sst-autotester
Copy link
Contributor

sst-autotester commented Apr 2, 2025

Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging
NO INSPECTION HAS BEEN PERFORMED ON THIS PULL REQUEST! - This PR must be inspected by setting label 'AT: PRE-TEST INSPECTED'.

@sst-autotester
Copy link
Contributor

sst-autotester commented Apr 2, 2025

Status Flag 'Pre-Test Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED by label AT: PRE-TEST INSPECTED! Autotester is Removing Label; this inspection will remain valid until a new commit to source branch is performed.

@sst-autotester
Copy link
Contributor

sst-autotester commented Apr 2, 2025

Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects:

Pull Request Auto Testing STARTING (click to expand)

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements

  • Build Num: 1924
  • Status: STARTED

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements_MR-2

  • Build Num: 1880
  • Status: STARTED

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements_MT-2

  • Build Num: 1879
  • Status: STARTED

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-macro_withsstcore

  • Build Num: 846
  • Status: STARTED

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-core_Make-Dist

  • Build Num: 699
  • Status: STARTED

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_Clang-Format_sst-core

  • Build Num: 653
  • Status: STARTED

Build Information

Test Name: SST__AutotestGen2_NewFW_OSX-14-XC15-ARM2_OMPI-4.1.6_PY3.10_sst-elements

  • Build Num: 447
  • Status: STARTED

Build Information

Test Name: SST__AutotestGen2_NewFW_OSX-14-XC15-ARM2_OMPI-4.1.6_PY3.10_sst-macro_withsstcore

  • Build Num: 271
  • Status: STARTED

Using Repos:

Repo: CORE (leekillough/sst-core)
  • Branch: valgrind_errors
  • SHA: 9441945
  • Mode: TEST_REPO
Repo: SQE (sstsimulator/sst-sqe)
  • Branch: devel
  • SHA: e9cae095c2c8936324608f4f994c3f04d7e0d992
  • Mode: SUPPORT_REPO
Repo: ELEMENTS (sstsimulator/sst-elements)
  • Branch: devel
  • SHA: bfa274ef54880d5172d7f70fc55835238a486250
  • Mode: SUPPORT_REPO
Repo: MACRO (sstsimulator/sst-macro)
  • Branch: devel
  • SHA: 42e85e1689d473c65fdbcc008ce57fd53fe80865
  • Mode: SUPPORT_REPO

Pull Request Author: leekillough

@sst-autotester
Copy link
Contributor

sst-autotester commented Apr 2, 2025

Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED

Pull Request Auto Testing has PASSED (click to expand)

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements

  • Build Num: 1924
  • Status: PASSED

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements_MR-2

  • Build Num: 1880
  • Status: PASSED

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-elements_MT-2

  • Build Num: 1879
  • Status: PASSED

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-macro_withsstcore

  • Build Num: 846
  • Status: PASSED

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_OMPI-4.1.4_PY3.6_sst-core_Make-Dist

  • Build Num: 699
  • Status: PASSED

Build Information

Test Name: SST__AutotestGen2_NewFW_sst-test_Clang-Format_sst-core

  • Build Num: 653
  • Status: PASSED

Build Information

Test Name: SST__AutotestGen2_NewFW_OSX-14-XC15-ARM2_OMPI-4.1.6_PY3.10_sst-elements

  • Build Num: 447
  • Status: PASSED

Build Information

Test Name: SST__AutotestGen2_NewFW_OSX-14-XC15-ARM2_OMPI-4.1.6_PY3.10_sst-macro_withsstcore

  • Build Num: 271
  • Status: PASSED

@sst-autotester
Copy link
Contributor

sst-autotester commented Apr 2, 2025

Status Flag 'Pre-Merge Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED AND APPROVED by [ feldergast ]!

@sst-autotester
Copy link
Contributor

sst-autotester commented Apr 2, 2025

Status Flag 'Pull Request AutoTester' - Pull Request MUST BE MERGED MANUALLY BY Project Team - This Repo does not support Automerge

4 similar comments
@sst-autotester
Copy link
Contributor

sst-autotester commented Apr 2, 2025

Status Flag 'Pull Request AutoTester' - Pull Request MUST BE MERGED MANUALLY BY Project Team - This Repo does not support Automerge

@sst-autotester
Copy link
Contributor

sst-autotester commented Apr 3, 2025

Status Flag 'Pull Request AutoTester' - Pull Request MUST BE MERGED MANUALLY BY Project Team - This Repo does not support Automerge

@sst-autotester
Copy link
Contributor

sst-autotester commented Apr 3, 2025

Status Flag 'Pull Request AutoTester' - Pull Request MUST BE MERGED MANUALLY BY Project Team - This Repo does not support Automerge

@sst-autotester
Copy link
Contributor

sst-autotester commented Apr 3, 2025

Status Flag 'Pull Request AutoTester' - Pull Request MUST BE MERGED MANUALLY BY Project Team - This Repo does not support Automerge

@feldergast feldergast merged commit f680243 into sstsimulator:devel Apr 3, 2025
7 checks passed
@berquist berquist added this to the SST V15.0.0 milestone Apr 14, 2025
@berquist berquist mentioned this pull request Apr 14, 2025
Closed
@leekillough leekillough deleted the valgrind_errors branch May 11, 2025 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@feldergast feldergast feldergast approved these changes

Assignees

No one assigned

Labels

AT: CLANG-FORMAT PASS AT: CMAKE-FORMAT PASS

Projects

None yet

Milestone

SST V15.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@leekillough @sst-autotester @feldergast @berquist