Standardise interface for accessing the owner of a symbol #4455
Conversation
Without the functions added by this commit, the only way to access the owner of a symbol for static fields and java methods is through `symbol.type.get(ID_C_class)`. When this technique is used by downstream repositories, they are vulnerable to being broken by upstream changes to how this information is stored in JBMC. The intension of this commit is that the two new functions added will be the standard interface to this symbol -> owner link. This means that future changes to how this information is stored can be made without breaking downstream repositories by changing these two functions.
This commit replaces access to `symbol.type.get(ID_C_class)`. With calls to the `owning_class` function and access to `symbol.type.set(ID_C_class, class_id)` with calls to `set_owning_class`. This allows for changes to how the owner of a symbol is stored by changing these two functions only, rather than by changing all the places where `ID_C_class` was previously accessed directly. This is preparatory work for moving this link from being stored in a comment on a symbols type, to the symbol itself.
thomasspriggs
requested review from
cesaro,
cristina-david,
NathanJPhillips,
peterschrammel,
pkesseli,
smowton and
thk123
as code owners
There was a problem hiding this comment.
What is meant by the "owner of a symbol?" For a given symbol x, Is it the class that x is a member of? But then those aren't separate symbols, they would just be members of a struct type. So I'm confused.
Edit: the only owner of a symbol that we have is the symbol table.
| @@ -419,6 +419,8 @@ void java_bytecode_convert_methodt::convert( | |||
| get_method_identifier(class_symbol.name, m); | |||
|
|
|||
| method_id=method_identifier; | |||
| set_owning_class( | |||
There was a problem hiding this comment.
⛏️ Lower down (line 537) you have a writeable ref for this symbol already which is used to update the type. It might be nicer to keep the mutation in one place and do this call down there
There was a problem hiding this comment.
I tried putting it lower down and things didn't work properly. I guess it must be read back somewhere between the two locations.
tautschnig
dismissed
their stale review
Discussed offline how the terminology might be changed.
There was a problem hiding this comment.
This PR failed Diffblue compatibility checks (cbmc commit: bd2bacd).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/106300980
Status will be re-evaluated on next push.
Common spurious failures include: the cbmc commit has disappeared in the mean time (e.g. in a force-push); the author is not in the list of contributors (e.g. first-time contributors); compatibility was already broken by an earlier merge.
|
I have
|
There was a problem hiding this comment.
This PR failed Diffblue compatibility checks (cbmc commit: 65e3d31).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/106341108
Status will be re-evaluated on next push.
Common spurious failures include: the cbmc commit has disappeared in the mean time (e.g. in a force-push); the author is not in the list of contributors (e.g. first-time contributors); compatibility was already broken by an earlier merge.
Without the functions added by this PR, the only way to access the
owner of a symbol for static fields and java methods is through
symbol.type.get(ID_C_class). When this technique is used by downstreamrepositories, they are vulnerable to being broken by upstream changes to
how this information is stored in JBMC. The intention of this PR is
that the two new functions added will be the standard interface to this
symbol -> owner link. This means that future changes to how this
information is stored can be made without breaking downstream
repositories by changing these two functions.
This PR includes changes to use the two new functions. This is preparatory
work ready for moving the link from being stored in a comment
on a symbols type, to the symbol itself, as discussed with @smowton