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.

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

LLL: alloc issues round-up #2545

Merged
merged 1 commit into from
Jul 11, 2017
Merged

Conversation

benjaminion
Copy link
Contributor

@benjaminion benjaminion commented Jul 9, 2017

This is an attempt to move the LLL alloc issue on. It supersedes PRs #2489 and #2463, and resolves Issue #2465.

The proposed change to the generated bytecode is the tightest code I can make that handles both the edge cases (alloc 0) and MSIZE being initially zero.

Notes:

A bunch of test cases is included that should be comprehensive.

Fixes #2465.

@benjaminion
Copy link
Contributor Author

Note that, alternatively, alloc could be implemented pretty efficiently as a compiler macro as follows:

  (def 'alloc (n) (raw (msize) (when n (pop (mload (+ (msize) (& (- n 1) (~ 0x1f))))))))

The downside of this is that it removes the opportunity to do memory management between alloc and variables - which seems to be the purpose of the finalise function. The memory management is not currently working, so as it stands it makes no practical difference if we do alloc in the parser or in the macros. But if we want to do the memory management in future, alloc ought to be left in the parser where it currently is.

{
char const* sourceCode = R"(
(returnlll
(return (- (msize) (alloc (calldataload 0x04)))))
Copy link
Member

@axic axic Jul 11, 2017

Choose a reason for hiding this comment

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

This is (msize) - (alloc..), right? So the resulting value should be how much was allocated.

Can you please add that as a comment somewhere?

Copy link
Member

Choose a reason for hiding this comment

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

What is weird though it assumes that execution goes as:

  • alloc
  • msize
  • sub

But the compiler could actually execute the two operands in a different order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm. Good spot. I've updated it to be resistant to changes in argument evaluation order. Ugly but effective.

(returnlll
(seq
(mstore 0x00 0) ; reserve space for the result of the alloc
(mstore 0x00 (alloc (calldataload 0x04)))
Copy link
Member

Choose a reason for hiding this comment

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

Indentation seems to be off here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aaaagh... must fix my editor...

@benjaminion benjaminion force-pushed the lll-alloc-updated branch 3 times, most recently from 457f5ca to 057c165 Compare July 11, 2017 16:57
@axic
Copy link
Member

axic commented Jul 11, 2017

My only nitpicking is I like comments above statements (as opposed to next to them), but I guess it is more readable in this case, since it fits a single window.

Please include your macro version as a comment in the macro section so it is not forgotten. Let's keep the native version for now and see if we can/should fix finalise.

@@ -523,14 +523,24 @@ void CodeFragment::constructOperation(sp::utree const& _t, CompilerState& _s)
requireSize(1);
requireDeposit(0, 1);

m_asm.append(Instruction::MSIZE);
Copy link
Member

Choose a reason for hiding this comment

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

Please add a comment here explaining the rules:

  • returns msize prior to expansion
  • doesn't allocate in case of N = 0
  • rounds N to a multiple of 32
  • uses MLOAD to expand, which keeps memory untouched

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@benjaminion
Copy link
Contributor Author

My only nitpicking is I like comments above statements (as opposed to next to them), but I guess it is more readable in this case, since it fits a single window.

Comments were mainly for my benefit; happy to keep them, move them, leave them...

Please include your macro version as a comment in the macro section so it is not forgotten.

Done. I put it at the top as some of the other macros could probably usefully be rewritten to use ALLOC now that it is reliable.

I've squashed the commits into one for convenience.

By the way, I wrote some docs.

@axic axic merged commit 699a372 into ethereum:develop Jul 11, 2017
@benjaminion benjaminion deleted the lll-alloc-updated branch July 12, 2017 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LLL: (alloc 0) corrupts memory
2 participants