sem/transf: don't collapse implict addr/deref nodes #498
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
saem
approved these changes
Dec 17, 2022
compiler/sem/semexprs.nim
Outdated
| else: | ||
| result = c.config.newError(n, reportAst(rsemVarForOutParamNeeded, n)) | ||
| ## Wraps the expression `n` in an ``nkHiddenAddr`` or, if the expression is | ||
| ## not a mutable lvalue, an ``nkError`` |
There was a problem hiding this comment.
Thanks for documenting it, as well.
| let n = result | ||
| # collapse ``nkAddr( nkDerefExpr(x) )`` to ``x``, but only if ``x`` is of | ||
| # pointer type. Performing the collapsing when ``x`` is a ``ref`` would be | ||
| # incorrect, as there'd be a formal vs. actual type mismatch then |
zerbina
force-pushed
the
no-addr-collapsing
branch
from
3390d1f to
badb6e8
Compare
|
Once tests pass let's merge. |
haxscramper
added
refactor
haxscramper
approved these changes
Dec 18, 2022
zerbina
force-pushed
the
no-addr-collapsing
branch
2 times, most recently
from
e7ae3e5 to
4742dad
Compare
This was referenced Dec 21, 2022
zerbina
force-pushed
the
no-addr-collapsing
branch
from
b4f136a to
243dcb7
Compare
|
Another detour is needed, it seems. The PR that fixed the JS code-generator issue that was the previous blocker also added a test for arguments to |
Clyybber
approved these changes
Dec 24, 2022
## Summary The collapsing produces unsound AST in the case that the operand of the `nkHiddenDeref` doesn't have the same type as the `nkHiddenAddr` node. For example, if the `nkHiddenDeref` dereferences a `ptr` or `ref` value, the collapsing would result in a `ptr` or `ref` value to be directly passed to `var` parameter (because `nkHiddenAddr` nodes are generated when passing something to a `var` parameter), which is incorrect. The reason that this didn't cause many problems so far is that `var`, `ref`, and `ptr` are all pointers underneath for the C target, and that `cgen` doesn't inspect the type of nodes in many cases. The collapsing done directly in `semexpr` is removed and the one done in `transf` is adjusted: - only `DerefExpr( Addr(x) )` and `Addr( DerefExpr(x) )` (both are expressions explicitly provided by the user) are collapsed -- the latter one only when `x` is of `ptr` type. - since collapsing `HiddenDeref( HiddenAddr(x) )` is always known to be safe, the behaviour is kept. Because of a `cgen` issue, it's currently also required. Unconditional collapsing of all address-of + deref expression is now performed inside the C code-generator, because it is known that the operands to all dereference operations map to C pointers at that point. ## Details The compiler targets C89, where dereferencing a pointer requires a pointer to complete type, even when the result is immediately passed to an address-of operation (i.e. `&(*x)`). `cgen` now requests the complete type whenever emitting a deref. `vmgen` depends on the removed collapsing and thus generates incorrect code in some cases now -- until the code-generator is fixed, the `tvar_arguments` test is disabled for the VM target.
zerbina
force-pushed
the
no-addr-collapsing
branch
from
243dcb7 to
55a8ca7
Compare
|
I'm in the process of fixing the mentioned In order to break the cyclic dependency, I disabled the bors r+ |
|
Build succeeded: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
The collapsing produces unsound AST in the case that the operand of the
nkHiddenDerefdoesn't have the same type as thenkHiddenAddrnode.For example, if the
nkHiddenDerefdereferences aptrorrefvalue,the collapsing would result in a
ptrorrefvalue to be directlypassed to
varparameter (becausenkHiddenAddrnodes are generatedwhen passing something to a
varparameter), which is incorrect.The reason that this didn't cause many problems so far is that
var,ref, andptrare all pointers underneath for the C target, and thatcgendoesn't inspect the type of nodes in many cases.The collapsing done directly in
semexpris removed and the one done intransfis adjusted:DerefExpr( Addr(x) )andAddr( DerefExpr(x) )(both areexpressions explicitly provided by the user) are collapsed -- the
latter one only when
xis ofptrtype.HiddenDeref( HiddenAddr(x) )is always known to besafe, the behaviour is kept. Because of a
cgenissue, it's currentlyalso required.
Unconditional collapsing of all address-of + deref expression is now
performed inside the C code-generator, because it is known that the
operands to all dereference operations map to C pointers at that point.
Details
The compiler targets C89, where dereferencing a pointer requires a
pointer to complete type, even when the result is immediately passed to
an address-of operation (i.e.
&(*x)).cgennow requests the completetype whenever emitting a deref.