fix: constructor context for internal functions #3388

charles-cooper · 2023-05-08T01:08:46Z

What I did

How I did it

add constructor context to codegen, skip cleverness with called_functions

How to verify it

Commit message

this commit fixes two related issues with initcode generation:

- nested internal functions called from the constructor would cause a
  compiler panic
- internal functions called from the constructor would not read/write
  from the correct immutables space

the relevant examples reproducing each issue are in the tests. this
commit fixes the issue by

- not trying to traverse the call graph to figure out which internal
  functions to include in the initcode. instead, all internal functions
  are included, and we rely on the dead code eliminator to remove unused
  functions
- adding a "constructor" flag to the codegen, so we can distinguish
  between internal calls which are being generated to include in
  initcode or runtime code.

Description for the changelog

Cute Animal Picture

fix incorrect codegen (using dload instead of iload) fix non-recursing for called functions from ctor

codecov-commenter · 2023-05-08T01:31:48Z

Codecov Report

Merging #3388 (a0d43dc) into master (3c83947) will decrease coverage by 0.14%.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master    #3388      +/-   ##
==========================================
- Coverage   88.92%   88.79%   -0.14%     
==========================================
  Files          86       86              
  Lines       10726    10771      +45     
  Branches     2440     2447       +7     
==========================================
+ Hits         9538     9564      +26     
- Misses        787      809      +22     
+ Partials      401      398       -3

Impacted Files	Coverage Δ
vyper/codegen/context.py	`91.85% <100.00%> (-0.06%)`	⬇️
vyper/codegen/expr.py	`87.50% <100.00%> (-0.10%)`	⬇️
vyper/codegen/function_definitions/common.py	`100.00% <100.00%> (ø)`
vyper/codegen/module.py	`97.75% <100.00%> (+0.05%)`	⬆️

... and 13 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

this commit fixes two issues in constructor codegen: - nested calls to internal functions (i.e. the constructor calls a() which then calls b()) were not compiling properly, only a single level of internal calls were tracked correctly, and - internal functions which used immutables were being produced with the incorrect `dload` instead of `iload` instruction. it fixes the first issue by simply including the code for all functions in the generated IR for the constructor and depending on the dead code eliminator to eliminate unused functions - instead of trying to traverse the call graph and only including called functions. the second issue is fixed by producing a separate compilation context for codegen which indicates whether the function is being called from the constrcutor or not - and depending on the context, the correct instruction can be emitted.

pcaversaccio · 2023-05-09T16:30:32Z

@charles-cooper can confirm that this fixes my issue. I tested it using my snekmate ERC20 implementation by replacing:

    if (initial_supply != empty(uint256)):
        self._before_token_transfer(empty(address), msg.sender, initial_supply)
        self.totalSupply = initial_supply
        self.balanceOf[msg.sender] = initial_supply
        log Transfer(empty(address), msg.sender, initial_supply)
        self._after_token_transfer(empty(address), msg.sender, initial_supply)

with

    if (initial_supply != empty(uint256)):
        self._mint(msg.sender, initial_supply)

where _mint itself calls another internal functions:

@internal
def _mint(owner: address, amount: uint256):
    """
    @dev Creates `amount` tokens and assigns
         them to `owner`, increasing the
         total supply.
    @notice This is an `internal` function without
            access restriction. Note that `owner`
            cannot be the zero address.
    @param owner The 20-byte owner address.
    @param amount The 32-byte token amount to be created.
    """
    assert owner != empty(address), "ERC20: mint to the zero address"

    self._before_token_transfer(empty(address), owner, amount)

    self.totalSupply += amount
    self.balanceOf[owner] = unsafe_add(self.balanceOf[owner], amount)
    log Transfer(empty(address), owner, amount)

    self._after_token_transfer(empty(address), owner, amount)

Everything compiles smoothly :- D! thx.

tests/parser/features/test_init.py

pcaversaccio · 2023-05-15T08:25:23Z

tests/parser/features/test_init.py

do you think it's worth adding a separate test that combines both issues? Also, it might make sense to add an additional test where the immutable variable is declared as public since this involves the generation of an additional getter that also must read the state correctly.

added the addl test in ddbeed0

pcaversaccio · 2023-05-15T08:35:30Z

vyper/codegen/module.py

+            func_ir = generate_ir_for_function(
+                f, all_sigs, global_ctx, skip_nonpayable_check=False, is_ctor_context=True
+            )
+            # note: we depend on dead code eliminator to clean dead function defs


So are you 100% sure that if internal functions run only within ctor are eliminated by the dead code eliminator or does this need further investigations? Can we maybe add here an additional test? Just to ensure to no dead code is deployed (this happened to Solidity already btw).

yea i'm 100% sure, i double checked by hand. we could add tests for the dead code eliminator altho it seems a bit out of scope for this PR

ps:

@internal def foo(): pass @internal def qux(): pass @external def bar(): self.foo() @external def __init__(): self.qux()

produces the following asm:

CALLVALUE _sym_revert1 JUMPI _sym_external___init______cleanup _sym_internal_qux____cleanup JUMP _sym_external___init______cleanup JUMPDEST _sym_subcode_size _sym_runtime_begin2 _mem_deploy_start CODECOPY _OFST _sym_subcode_size 0 _mem_deploy_start RETURN _sym_runtime_begin2 BLANK { _DEPLOY_MEM_OFST_64 PUSH1 0x00 CALLDATALOAD PUSH1 0xe0 SHR CALLVALUE _sym_revert1 JUMPI PUSH4 0xfebb0f7e DUP2 XOR _sym_join3 JUMPI PUSH1 0x04 CALLDATASIZE LT _sym_revert1 JUMPI _sym_external_bar____cleanup _sym_internal_foo____cleanup JUMP _sym_external_bar____cleanup JUMPDEST STOP _sym_join3 JUMPDEST POP PUSH1 0x00 PUSH1 0x00 REVERT _sym_internal_foo____cleanup JUMPDEST JUMP _sym_revert1 JUMPDEST PUSH1 0x00 DUP1 REVERT } _sym_internal_qux____cleanup JUMPDEST JUMP _sym_revert1 JUMPDEST PUSH1 0x00 DUP1 REVERT

which you can manually verify that qux does not appear in the runtime code and foo does not appear in the initcode.

ok - i added the dead code eliminator test in 2897ff4

I also quickly verified it manually. Also, for the record, I tested this morning manually a contract with 10 nested internal calls. +1 for the code eliminator test.

…tions

pcaversaccio · 2023-05-15T16:38:07Z

LGTM now!

fubuloubu

Is there a test for an internal that's in both the constructor and a public function?

charles-cooper · 2023-05-15T18:11:11Z

Is there a test for an internal that's in both the constructor and a public function?

will add

charles-cooper · 2023-05-15T18:19:34Z

Is there a test for an internal that's in both the constructor and a public function?

see a0d43dc

charles-cooper added 2 commits May 7, 2023 17:59

fix: constructor callees

bb1b037

fix incorrect codegen (using dload instead of iload) fix non-recursing for called functions from ctor

add tests

af15d43

charles-cooper requested a review from fubuloubu May 8, 2023 01:08

charles-cooper mentioned this pull request May 11, 2023

refactor: remove FunctionSignature #3390

Merged

charles-cooper changed the title ~~fix: ctor issues~~ fix: initcode codegen bugs May 12, 2023

pcaversaccio reviewed May 15, 2023

View reviewed changes

charles-cooper added 4 commits May 15, 2023 11:57

add test for dead code eliminator

2897ff4

fix some comments

bbfaf2a

rename some "issue" tags

c52449c

move a test, and add test for immutable reads in nested internal func…

ddbeed0

…tions

fubuloubu approved these changes May 15, 2023

View reviewed changes

add test to check immutable read from both ctor and runtime

a0d43dc

fubuloubu approved these changes May 15, 2023

View reviewed changes

charles-cooper enabled auto-merge (squash) May 15, 2023 18:31

charles-cooper changed the title ~~fix: initcode codegen bugs~~ fix: immutables behavior in internal functions called from constructor May 15, 2023

charles-cooper disabled auto-merge May 15, 2023 18:31

charles-cooper changed the title ~~fix: immutables behavior in internal functions called from constructor~~ fix: constructor context for internal functions May 15, 2023

charles-cooper enabled auto-merge (squash) May 15, 2023 18:37

charles-cooper merged commit c202c4e into vyperlang:master May 15, 2023

charles-cooper deleted the fix/ctor_issues branch May 15, 2023 19:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: constructor context for internal functions #3388

fix: constructor context for internal functions #3388

charles-cooper commented May 8, 2023 •

edited

Loading

codecov-commenter commented May 8, 2023 •

edited

Loading

pcaversaccio commented May 9, 2023

pcaversaccio May 15, 2023

charles-cooper May 15, 2023

pcaversaccio May 15, 2023

charles-cooper May 15, 2023 •

edited

Loading

charles-cooper May 15, 2023

pcaversaccio May 15, 2023

pcaversaccio commented May 15, 2023

fubuloubu left a comment

charles-cooper commented May 15, 2023

charles-cooper commented May 15, 2023

fix: constructor context for internal functions #3388

fix: constructor context for internal functions #3388

Conversation

charles-cooper commented May 8, 2023 • edited Loading

What I did

How I did it

How to verify it

Commit message

Description for the changelog

Cute Animal Picture

codecov-commenter commented May 8, 2023 • edited Loading

Codecov Report

pcaversaccio commented May 9, 2023

pcaversaccio May 15, 2023

Choose a reason for hiding this comment

charles-cooper May 15, 2023

Choose a reason for hiding this comment

pcaversaccio May 15, 2023

Choose a reason for hiding this comment

charles-cooper May 15, 2023 • edited Loading

Choose a reason for hiding this comment

charles-cooper May 15, 2023

Choose a reason for hiding this comment

pcaversaccio May 15, 2023

Choose a reason for hiding this comment

pcaversaccio commented May 15, 2023

fubuloubu left a comment

Choose a reason for hiding this comment

charles-cooper commented May 15, 2023

charles-cooper commented May 15, 2023

charles-cooper commented May 8, 2023 •

edited

Loading

codecov-commenter commented May 8, 2023 •

edited

Loading

charles-cooper May 15, 2023 •

edited

Loading