Skip to content
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

Jump to bottom

[EraVM] Fix: handle duplicating library symbols #729

Merged
merged 1 commit into from
Oct 31, 2024
Merged

[EraVM] Fix: handle duplicating library symbols #729

merged 1 commit into from
Oct 31, 2024

Conversation

PavelKopyl
Copy link
Contributor

There may be an arbitrary number of 'Linkersymbol' instructions with the same argument. Map them to the unique MCSymbol.

Code Review Checklist

Purpose

Ticket Number

Requirements

  • Have the requirements been met?
  • Have stakeholder(s) approved the change?

Implementation

  • Does this code change accomplish what it is supposed to do?
  • Can this solution be simplified?
  • Does this change add unwanted compile-time or run-time dependencies?
  • Could an additional framework, API, library, or service improve the solution?
  • Could we reuse part of LLVM instead of implementing the patch or a part of it?
  • Is the code at the right abstraction level?
  • Is the code modular enough?
  • Can a better solution be found in terms of maintainability, readability, performance, or security?
  • Does similar functionality already exist in the codebase? If yes, why isn’t it reused?
  • Are there any best practices, design patterns or language-specific patterns that could substantially improve this code?

Logic Errors and Bugs

  • Can you think of any use case in which the
    code does not behave as intended?
  • Can you think of any inputs or external events
    that could break the code?

Error Handling and Logging

  • Is error handling done the correct way?
  • Should any logging or debugging information
    be added or removed?
  • Are error messages user-friendly?
  • Are there enough log events and are they
    written in a way that allows for easy
    debugging?

Maintainability

  • Is the code easy to read?
  • Is the code not repeated (DRY Principle)?
  • Is the code method/class not too long?

Dependencies

  • Were updates to documentation, configuration, or readme files made as required by this change?
  • Are there any potential impacts on other parts of the system or backward compatibility?

Security

  • Does the code introduce any security vulnerabilities?

Performance

  • Do you think this code change decreases
    system performance?
  • Do you see any potential to improve the
    performance of the code significantly?

Testing and Testability

  • Is the code testable?
  • Have automated tests been added, or have related ones been updated to cover the change?
    • For changes to mutable state
  • Do tests reasonably cover the code change (unit/integration/system tests)?
    • Line Coverage
    • Region Coverage
    • Branch Coverage
  • Are there some test cases, input or edge cases
    that should be tested in addition?

Readability

  • Is the code easy to understand?
  • Which parts were confusing to you and why?
  • Can the readability of the code be improved by
    smaller methods?
  • Can the readability of the code be improved by
    different function, method or variable names?
  • Is the code located in the right
    file/folder/package?
  • Do you think certain methods should be
    restructured to have a more intuitive control
    flow?
  • Is the data flow understandable?
  • Are there redundant or outdated comments?
  • Could some comments convey the message
    better?
  • Would more comments make the code more
    understandable?
  • Could some comments be removed by making the code itself more readable?
  • Is there any commented-out code?
  • Have you run a spelling and grammar checker?

Documentation

  • Is there sufficient documentation?
  • Is the ReadMe.md file up to date?

Best Practices

  • Follow Single Responsibility principle?
  • Are different errors handled correctly?
  • Are errors and warnings logged?
  • Magic values avoided?
  • No unnecessary comments?
  • Minimal nesting used?

Experts' Opinion

  • Do you think a specific expert, like a security
    expert or a usability expert, should look over
    the code before it can be accepted?
  • Will this code change impact different teams, and should they review the change as well?

@PavelKopyl PavelKopyl changed the title [EraVM] Fix: handle duplicating library symbols. [DO NO MERGE][EraVM] Fix: handle duplicating library symbols. Oct 31, 2024
@PavelKopyl PavelKopyl force-pushed the kpv-eravm-fix-linker-duplicating-symbols branch from 0b8adf3 to 662baf2 Compare October 31, 2024 00:51
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 31, 2024 •
edited
Loading 

╔═╡ Size (-%) ╞════════════════╡ All M3B3 ╞═╗
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                               0.000 ║
╠═╡ Cycles (-%) ╞══════════════╡ All M3B3 ╞═╣
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                               0.000 ║
╠═╡ Ergs (-%) ╞════════════════╡ All M3B3 ╞═╣
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                               0.000 ║
╠══╡ Gas (-%) ╞════════════════╡ All M3B3 ╞═╣
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                               0.000 ║
╚═══════════════════════════════════════════╝

╔═╡ Size (-%) ╞════════════════╡ All MzB3 ╞═╗
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                               0.000 ║
╠═╡ Cycles (-%) ╞══════════════╡ All MzB3 ╞═╣
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                               0.000 ║
╠═╡ Ergs (-%) ╞════════════════╡ All MzB3 ╞═╣
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                               0.000 ║
╠══╡ Gas (-%) ╞════════════════╡ All MzB3 ╞═╣
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                               0.000 ║
╚═══════════════════════════════════════════╝

╔═╡ Size (-%) ╞═════╡ EVMInterpreter M3B3 ╞═╗
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                                 NaN ║
╠═╡ Cycles (-%) ╞═══╡ EVMInterpreter M3B3 ╞═╣
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                               0.000 ║
╠═╡ Ergs (-%) ╞═════╡ EVMInterpreter M3B3 ╞═╣
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                               0.000 ║
╠══╡ Gas (-%) ╞═════╡ EVMInterpreter M3B3 ╞═╣
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                               0.000 ║
╠═╡ Ergs/gas ╞══════╡ EVMInterpreter M3B3 ╞═╣
║ ADD                                28.083 ║
║ MUL                                16.850 ║
║ SUB                                28.083 ║
║ DIV                                19.250 ║
║ SDIV                               36.050 ║
║ MOD                                19.250 ║
║ SMOD                               33.650 ║
║ ADDMOD                             18.156 ║
║ MULMOD                             16.656 ║
║ EXP                                 6.904 ║
║ SIGNEXTEND                         19.250 ║
║ LT                                 32.083 ║
║ GT                                 32.083 ║
║ SLT                                54.083 ║
║ SGT                                54.083 ║
║ EQ                                 32.083 ║
║ ISZERO                             27.750 ║
║ AND                                28.083 ║
║ OR                                 28.083 ║
║ XOR                                28.083 ║
║ NOT                                23.750 ║
║ BYTE                               38.083 ║
║ SHL                                34.083 ║
║ SHR                                34.083 ║
║ SAR                                52.083 ║
║ SGT                                54.083 ║
║ SHA3                               25.073 ║
║ ADDRESS                            41.812 ║
║ BALANCE                            20.924 ║
║ ORIGIN                           1351.625 ║
║ CALLER                             41.812 ║
║ CALLVALUE                          41.812 ║
║ CALLDATALOAD                       26.083 ║
║ CALLDATASIZE                       42.125 ║
║ CALLDATACOPY                       49.492 ║
║ CODESIZE                           45.625 ║
║ CODECOPY                           62.456 ║
║ GASPRICE                         1348.438 ║
║ EXTCODESIZE                         3.635 ║
║ EXTCODECOPY                         3.723 ║
║ RETURNDATASIZE                     43.500 ║
║ RETURNDATACOPY                     44.556 ║
║ EXTCODEHASH                         4.651 ║
║ BLOCKHASH                         238.819 ║
║ COINBASE                         1348.625 ║
║ TIMESTAMP                        1342.625 ║
║ NUMBER                           1342.625 ║
║ PREVRANDAO                       1342.625 ║
║ GASLIMIT                         1348.625 ║
║ CHAINID                          1336.625 ║
║ SELFBALANCE                       639.250 ║
║ BASEFEE                          1342.625 ║
║ POP                                39.125 ║
║ MLOAD                              43.667 ║
║ MSTORE                             55.248 ║
║ MSTORE8                            64.716 ║
║ SLOAD                              19.044 ║
║ SSTORE                              4.467 ║
║ JUMP                               15.667 ║
║ JUMPI                              15.636 ║
║ PC                                 42.312 ║
║ MSIZE                              48.812 ║
║ GAS                                42.312 ║
║ JUMPDEST                           59.625 ║
║ PUSH0                              42.312 ║
║ PUSH1                              37.958 ║
║ PUSH2                              43.375 ║
║ PUSH4                              46.208 ║
║ PUSH5                              47.625 ║
║ PUSH6                              49.042 ║
║ PUSH7                              50.458 ║
║ PUSH8                              51.875 ║
║ PUSH9                              53.292 ║
║ PUSH10                             54.708 ║
║ PUSH11                             56.125 ║
║ PUSH12                             57.542 ║
║ PUSH13                             58.958 ║
║ PUSH14                             60.375 ║
║ PUSH15                             61.792 ║
║ PUSH16                             63.208 ║
║ PUSH17                             64.625 ║
║ PUSH18                             66.042 ║
║ PUSH19                             67.458 ║
║ PUSH20                             68.875 ║
║ PUSH21                             70.292 ║
║ PUSH22                             71.708 ║
║ PUSH23                             73.125 ║
║ PUSH24                             74.542 ║
║ PUSH25                             75.958 ║
║ PUSH26                             77.375 ║
║ PUSH27                             78.792 ║
║ PUSH28                             80.208 ║
║ PUSH29                             81.625 ║
║ PUSH30                             83.042 ║
║ PUSH31                             84.458 ║
║ PUSH32                             85.875 ║
║ DUP1                               26.083 ║
║ DUP2                               32.417 ║
║ DUP3                               32.417 ║
║ DUP4                               32.417 ║
║ DUP5                               32.417 ║
║ DUP6                               32.417 ║
║ DUP7                               32.417 ║
║ DUP8                               32.417 ║
║ DUP9                               32.417 ║
║ DUP10                              32.417 ║
║ DUP11                              32.417 ║
║ DUP12                              32.417 ║
║ DUP13                              32.417 ║
║ DUP14                              32.417 ║
║ DUP15                              32.417 ║
║ DUP16                              32.417 ║
║ SWAP1                              32.417 ║
║ SWAP2                              32.417 ║
║ SWAP3                              32.417 ║
║ SWAP4                              32.417 ║
║ SWAP5                              32.417 ║
║ SWAP6                              32.417 ║
║ SWAP7                              32.417 ║
║ SWAP8                              32.417 ║
║ SWAP9                              32.417 ║
║ SWAP10                             32.417 ║
║ SWAP11                             32.417 ║
║ SWAP12                             32.417 ║
║ SWAP13                             32.417 ║
║ SWAP14                             32.417 ║
║ SWAP15                             32.417 ║
║ SWAP16                             32.417 ║
║ CALL                               34.125 ║
║ STATICCALL                         34.095 ║
║ DELEGATECALL                       33.175 ║
║ CREATE                              3.713 ║
║ CREATE2                             5.009 ║
║ RETURN                              1.000 ║
║ REVERT                              1.000 ║
╠═╡ Ergs/gas (-%) ╞═╡ EVMInterpreter M3B3 ╞═╣
╚═══════════════════════════════════════════╝

╔═╡ Size (-%) ╞═════╡ EVMInterpreter MzB3 ╞═╗
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                                 NaN ║
╠═╡ Cycles (-%) ╞═══╡ EVMInterpreter MzB3 ╞═╣
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                               0.000 ║
╠═╡ Ergs (-%) ╞═════╡ EVMInterpreter MzB3 ╞═╣
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                               0.000 ║
╠══╡ Gas (-%) ╞═════╡ EVMInterpreter MzB3 ╞═╣
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                               0.000 ║
╚═══════════════════════════════════════════╝

╔═╡ Size (-%) ╞════════╡ Precompiles M3B3 ╞═╗
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                               0.000 ║
╠═╡ Cycles (-%) ╞══════╡ Precompiles M3B3 ╞═╣
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                               0.000 ║
╠═╡ Ergs (-%) ╞════════╡ Precompiles M3B3 ╞═╣
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                               0.000 ║
╠══╡ Gas (-%) ╞════════╡ Precompiles M3B3 ╞═╣
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                                 NaN ║
╚═══════════════════════════════════════════╝

╔═╡ Size (-%) ╞════════╡ Precompiles MzB3 ╞═╗
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                               0.000 ║
╠═╡ Cycles (-%) ╞══════╡ Precompiles MzB3 ╞═╣
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                               0.000 ║
╠═╡ Ergs (-%) ╞════════╡ Precompiles MzB3 ╞═╣
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                               0.000 ║
╠══╡ Gas (-%) ╞════════╡ Precompiles MzB3 ╞═╣
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                                 NaN ║
╚═══════════════════════════════════════════╝

╔═╡ Size (-%) ╞══════════╡ Real life M3B3 ╞═╗
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                               0.000 ║
╠═╡ Cycles (-%) ╞════════╡ Real life M3B3 ╞═╣
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                               0.000 ║
╠═╡ Ergs (-%) ╞══════════╡ Real life M3B3 ╞═╣
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                               0.000 ║
╠══╡ Gas (-%) ╞══════════╡ Real life M3B3 ╞═╣
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                                 NaN ║
╚═══════════════════════════════════════════╝

╔═╡ Size (-%) ╞══════════╡ Real life MzB3 ╞═╗
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                               0.000 ║
╠═╡ Cycles (-%) ╞════════╡ Real life MzB3 ╞═╣
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                               0.000 ║
╠═╡ Ergs (-%) ╞══════════╡ Real life MzB3 ╞═╣
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                               0.000 ║
╠══╡ Gas (-%) ╞══════════╡ Real life MzB3 ╞═╣
║ Best                                0.000 ║
║ Worst                               0.000 ║
║ Total                                 NaN ║
╚═══════════════════════════════════════════╝

@PavelKopyl PavelKopyl force-pushed the kpv-eravm-fix-linker-duplicating-symbols branch from 662baf2 to df9a891 Compare October 31, 2024 11:04
@PavelKopyl PavelKopyl changed the title [DO NO MERGE][EraVM] Fix: handle duplicating library symbols. [EraVM] Fix: handle duplicating library symbols. Oct 31, 2024
@PavelKopyl PavelKopyl changed the title [EraVM] Fix: handle duplicating library symbols. [EraVM] Fix: handle duplicating library symbols Oct 31, 2024
@PavelKopyl
[EraVM] Fix: handle duplicating library symbols
3821da3 
There may be an arbitrary number of 'Linkersymbol' instructions
with the same argument. Map them to the unique MCSymbol.
@PavelKopyl PavelKopyl force-pushed the kpv-eravm-fix-linker-duplicating-symbols branch from df9a891 to 3821da3 Compare October 31, 2024 11:49
akiramenai
akiramenai approved these changes Oct 31, 2024
View reviewed changes
Copy link
Collaborator

@akiramenai akiramenai left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@hedgar2017 hedgar2017 merged commit a58b8f5 into main Oct 31, 2024
10 of 11 checks passed
@hedgar2017 hedgar2017 deleted the kpv-eravm-fix-linker-duplicating-symbols branch October 31, 2024 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hedgar2017 hedgar2017 hedgar2017 approved these changes

@akiramenai akiramenai akiramenai approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@PavelKopyl @akiramenai @hedgar2017