Fix leak-prone CustomData in stdMem interface.

[bwhitchurch]

Use std::unique_ptr to give ownership of CustomData to the CustomRequest and CustomResponse classes.

Ensures that the CustomData will have it's destructor called without becoming unreachable. (#1000)

[github-actions[bot]]

CLANG-FORMAT TEST - FAILED (on last commit):
Run > ./scripts/clang-format-test.sh using clang-format v12 to check formatting

[github-actions[bot]]

CMAKE-FORMAT TEST - PASSED

[github-actions[bot]]

CMAKE-FORMAT TEST - PASSED

[github-actions[bot]]

CLANG-FORMAT TEST - PASSED

[bwhitchurch]

This change does affect a couple places in sst-elements. I have a branch with the required changes ready to go, assuming this PR is successful.

[sst-autotester]

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'.

[gvoskuilen]

Does this compile with the current sst-elements/devel branch and pass the tests in that repo? If not, it will fail testing.

[bwhitchurch]

It does not. There are a couple lines of code in Miranda and MemHierarchy that use data directly.

[gvoskuilen]

I agree with the idea, it would be better to have the CustomRequest/Response explicitly take and hand off ownership including responsibility for delete. And if memH or any other elements are currently leaking CustomData, we need to fix that regardless. But, because this is a breaking change in the sst-core API, we're evaluating what we need to do for backwards compatibility.

[bwhitchurch]

Cool, I understand that the breaking change is undesirable. We caught this in our codebase and the giving responsibility to CustomRequest/Response ended up being a simpler matter than following the Data objects and adding logic to various handlers to decide whether a delete needs to occur. I'll continue to watch for updates, please let me know if there is anything additional requested/needed to get this pulled in.

[gvoskuilen]

Thanks. We'll get this in once we figure out how to phase it in without breaking things. Feel free to propose a way to do that if you think of one.

Related, in digging through memH, it looks like there is a potential memory leak of CustomData in its own internal events, so I will fix that.

[bwhitchurch]

I have given this a little bit of thought and I think the following steps comprise a roadmap that doesn't break anything along the way.

First, we can try deleting data in the destructor of CustomRequest/Response. This may not be compatible with sst-elements though, iirc there is a shallow copy (move) done in memH and I don't recall if the source gets set to nullptr before the request lifetime ends. If it works it's a decent enough patch for current code.

To make the change to a unique_ptr:

1. Introduce a get() method CustomRequest/Response to access data.
2. Update sst-elements to use the get() method instead of req->data;
3. Change data to unique_ptr and possibly make it private, change CustomRequest::get() to return data.get().
4. (future future goals) change interfaces that take CustomData* to take std::unique_ptr<CustomData>& to use unique_ptr's ownership and move semantics.