search

ethereum / solidity

Solidity, the Smart Contract Programming Language
https://soliditylang.org
GNU General Public License v3.0
23.13k stars 5.74k forks source link

replace YulString with YulName typedef #15083

Closed clonker closed 2 months ago

clonker commented 4 months ago

in preparation of type change of YulString s.t. these changes are more localized

nikola-matic commented 4 months ago

What's this about? Is this regarding the Yul AST rework to get rid of the AST ID dependencies (or to at least strengthen them) in order then get rid of codegen non-determinism? Or is it something else?

clonker commented 4 months ago

The idea is to get rid of YulString as a kind of a leightweight string proxy in favor of a simple incremental uint64 index for handles (with a backing string vector of names) and do the string generation of derived names post-hoc

nikola-matic commented 4 months ago

The idea is to get rid of YulString as a kind of a leightweight string proxy in favor of a simple incremental uint64 index for handles (with a backing string vector of names) and do the string generation of derived names post-hoc

Alright, so killing the non-determinism in its root.

clonker commented 4 months ago

Depends on the level at which you're looking - but sure, restarting the optimization with an intermediate will kill determinism with that approach unless we do reordering after each step (which may not be so bad, let's see)

clonker commented 4 months ago

Hi @bshastry, as this changes some stuff in the fuzzing department as well, does the PR mess things up on your end or is it fine? :)

bshastry commented 4 months ago

Hi @bshastry, as this changes some stuff in the fuzzing department as well, does the PR mess things up on your end or is it fine? :)

Sorry for the late reply :-(

Since the ossfuzz build passed, I would assume it is fine with the fuzzer as well. Dynamic breakages (at runtime) need to be seen though, let me know if you want me to run the fuzzer on this PR. That should throw up issues if any!

clonker commented 4 months ago

Hi @bshastry, as this changes some stuff in the fuzzing department as well, does the PR mess things up on your end or is it fine? :)

Sorry for the late reply :-(

Since the ossfuzz build passed, I would assume it is fine with the fuzzer as well. Dynamic breakages (at runtime) need to be seen though, let me know if you want me to run the fuzzer on this PR. That should throw up issues if any!

Cheers, no worries. I expect this PR to be closed anyways and there's going to be a new one with a different set of changes. That will also impact the fuzzer, most likely. I'll let you know :)

github-actions[bot] commented 3 months ago

This pull request is stale because it has been open for 14 days with no activity. It will be closed in 7 days unless the stale label is removed.

github-actions[bot] commented 3 months ago

This pull request is stale because it has been open for 14 days with no activity. It will be closed in 7 days unless the stale label is removed.

github-actions[bot] commented 2 months ago

This pull request is stale because it has been open for 14 days with no activity. It will be closed in 7 days unless the stale label is removed.

clonker commented 2 months ago

In #15215 I have done the following to address the points you brought up:

I am in the process of breaking down that way too big PR into smaller (or if not small, at least hopefully trivial) ones ~and we can see about each of the points in more detail then, so as far as I am concerned, we don't have to merge this~.

clonker commented 2 months ago

Thanks for the feedback! I think my editor was playing some tricks on me with the formatting:)