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

Make assembler work better with code that issues *cblock opcodes. #4034

Closed
jannotti opened this issue May 23, 2022 · 1 comment · Fixed by #4442
Closed

Make assembler work better with code that issues *cblock opcodes. #4034

jannotti opened this issue May 23, 2022 · 1 comment · Fixed by #4442
Assignees
Labels
new-feature-request Feature request that needs triage Team Scytale

Comments

@jannotti
Copy link
Contributor

If a program has an explicit intcblock or bytecblock, the pseudops int and byte respectively have strange behavior.
The following program

        intcblock 2 3
        int 5

is assembled as:

#pragma version 1
intcblock 2 3
intc_2

Which will fail at runtime because the constant reference is beyond the constant block.

We could assemble pseudo-ops to the push form is they use a constant that is not in the user suppled constant block.

A slightly different problem. This program:

        intcblock 2 3
        intcblock 4 10
        int 5

somehow gets assembled to:

#pragma version 1
intcblock 2 3
intcblock 4 10
intc_2 // 4. (this disassembly is quite strange)

it's hard for me to guess what's going on. But the the 5 is again not going to work, as no intcblock is long enough for an index of 2.

We should probably use push* exclusively for the pseudo ops when two constant blocks appear. Presumably this would only be done by a clever compiler to TEAL, and it would use the intc,bytec ops explicitly to take advantage of its clever constant block usage.

A final problem, related to fixing the last problem. There are reasonable proposals to place a constant block at the end of programs as a place to put program metadata. It would be nice if doing so did not prevent the usual pseudo op assembly. That is, even though such programs have an explicit constant block, the normal assembly operation is fine.

@jannotti jannotti added the new-feature-request Feature request that needs triage label May 23, 2022
@jannotti
Copy link
Contributor Author

jannotti commented Aug 21, 2022

I now understand the problem and have a limited solution. The int pseudo op should mostly not be available if there is an explicit intcblock in the program. For one thing, there is the existing bug, in which the int pseudo op believes it can append to the the manual cblock, but it can not, because we do not modified explicit cblocks in TEAL source. Further, there are weird cases in which it's not even possible to know which cblock is in effect when a given instruction executes. So I advocate banning int in combination with intcblock. However, as a concession to some pending standardization efforts that are considering the use of the constant blocks as a way to convey metadata information about a program, we should try to allow "harmless" cblocks - for example, if a cblock is in deadcode, it can't cause problems.

(All of this is equally true of int constants and byte constants)

jannotti added a commit to jannotti/go-algorand that referenced this issue Aug 22, 2022
This PR fixes algorand#4034, by forcing the use of "push*" when mixing manual
constant blocks with the int/byte/addr/method pseudo-ops. (And a
related disassembly bug).

However, to avoid this "deoptimization" when manual blocks are used to
add program metadata, deadcode constant blocks are ignored for this
check.
jannotti added a commit that referenced this issue Sep 1, 2022
This PR fixes #4034, by forcing the use of "push*" when mixing manual
constant blocks with the int/byte/addr/method pseudo-ops. (And a
related disassembly bug).

However, to avoid this "deoptimization" when manual blocks are used to
add program metadata, deadcode constant blocks are ignored for this
check.
@michaeldiamant michaeldiamant reopened this Sep 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-feature-request Feature request that needs triage Team Scytale
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants