Skip to content

Free the map target when the object is destroyed #371

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Free the map target when the object is destroyed #371

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
2 changes: 2 additions & 0 deletions RELEASENOTES-1.4.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,3 +359,5 @@
- `note 6` Improved the log messages that are output by default for partial register accesses (fixes [#356](https://github.com/intel/device-modeling-language/issues/356)).
- `release 6 6385`
- `release 7 7080`
- `note 6` The `map_target` template from `utility.dml` now frees the internally
created map target when the object instantiating the template is deleted.
7 changes: 6 additions & 1 deletion lib/1.4/utility.dml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1238,7 +1238,7 @@ template bank_obj is (bank, name) {

The template defines the following methods:
*/
template map_target is (connect, _qname) {
template map_target is (connect, _qname, destroy) {
session map_target_t *map_target;

interface ram { param required = false; }
Expand Down Expand Up @@ -1272,6 +1272,11 @@ template map_target is (connect, _qname) {
this.map_target = obj ? SIM_new_map_target(obj, NULL, NULL) : NULL;
}

method destroy() default {
SIM_free_map_target(this.map_target);
this.map_target = NULL;
Comment on lines +1275 to +1277
Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding build errors: one thing you can do is to wrap this in a group _internal_destructor is destroy to avoid clashes.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is a solution, but is it desirable to always and unconditionally free the map target?

Copy link
Contributor Author

@TSonono TSonono May 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Regarding build errors: one thing you can do is to wrap this in a group _internal_destructor is destroy to avoid clashes.

Would that be preferred? I could fix these particular build errors in pcie-downstream-port and cxl-downstream-port in the base repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a solution, but is it desirable to always and unconditionally free the map target?

If the object is deleted, is there any scenario where not freeing the map target makes sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I was about to say. The build issues are due to various sporadic destroy_tmpl templates scattered across the repo that was a workaround before the destroy template was officially added. These could be addressed by simply cleaning up the old usages of destroy_tmpl and using destroy instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is a solution, but is it desirable to always and unconditionally free the map target?

If the object is deleted, is there any scenario where not freeing the map target makes sense?

Dunno. I figure not, save, peraps, some extremely bad exotic interactions, which would constitute separate bugs in their own right.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or well -- perhaps there'd be merit in making it an overridable method of the connect, because that'd allow the user to act on the connected object before freeing it (by calling default()). An internal destructor would make that impossible. But I don't know how realistic such a use case would be.

In general, the question of overrides are something to consider. If there is a destroy() specified, and the user doesn't call default() (whether they be because the code precedes this PR or because they forgot), then the map target won't get freed. Having an internal destructor object prevents that situation from occurring. Another consideration is that destroy of subobjects are guaranteed to be called first before their parent objects. This means there is no issue if there's a custom destroy of the connect that also frees the map target -- because this internal destructor will set the map_target to NULL, and SIM_free_map_target is a noop on NULL.

So there are pros and cons here. I'm leaning towards the internal destructor, though.

}

/**
* `read(uint64 addr, uint64 size) -> (uint64) throws`

Expand Down