Fixing parserUnroll bug for sequential next operator usage #3193

VolodymyrPeschanenkoIntel · 2022-04-08T09:48:48Z

These changes fix bug of parserUnroll which appears in issue561-6-bmv2.p4 example.
This example contains the following code:

state parseO1O1 {
        packet.extract(hdr.u.next.byte);
        packet.extract(hdr.u.next.byte);
        transition accept;
}

ParserUnroll generated the following state before these changes:

state parseO1O1 {
        packet.extract(hdr.u[0].byte);
        packet.extract(hdr.u[0].byte);  /* <= here should be hdr.u[1].byte, according to the semantics of a next operator. */
        transition accept;
}

fruffy · 2022-04-08T14:33:08Z

Any way to write a test for this? It will take a while until the loops unrolling pull request for p4test is in.

fruffy

Looks fine from my point of view. @mbudiu-vmw any comments?

fruffy · 2022-04-08T14:58:26Z

midend/parserUnroll.cpp

            }
            if (expression->member.name == IR::Type_Stack::lastIndex) {
                return new IR::Constant(IR::Type_Bits::get(32), idx);
            } else {
-                state->statesIndexes[expression->expr->toString()] = idx;
+                state->statesIndexes[expression->expr->toString()] = idx + offset;


expression->expr->toString() is there no better way to map this?

clone_id can't be applied here. It is possible to implement operator< for IR::Member, but IR::Member::member still contains cstring. So, for now, I think that this is the best solution.

node->toString() should only be used for reporting user messages.
Such usage in the compiler is unsafe, since the different expressions may generate identical toString outputs.

mihaibudiu · 2022-04-08T17:50:36Z

The spec does actually not mandate this behavior. The spec says that nextIndex should only be incremented if extracting directly to next. You can argue that this is a bug in the spec, but we should first take this change to the language design committee.

mihaibudiu · 2022-04-08T17:51:34Z

See https://p4.org/p4-spec/docs/P4-16-v1.2.2.html#sec-packet-extract-one

mihaibudiu · 2022-04-08T17:55:55Z

Filed p4lang/p4-spec#1048

mihaibudiu · 2022-04-08T17:57:30Z

midend/parserUnroll.cpp

            }
            if (expression->member.name == IR::Type_Stack::lastIndex) {
                return new IR::Constant(IR::Type_Bits::get(32), idx);
            } else {
-                state->statesIndexes[expression->expr->toString()] = idx;
+                state->statesIndexes[expression->expr->toString()] = idx + offset;


node->toString() should only be used for reporting user messages.
Such usage in the compiler is unsafe, since the different expressions may generate identical toString outputs.

VolodymyrPeschanenkoIntel · 2022-04-19T12:06:55Z

I added StackVariable class to support std::map<IR::Member, ...>

midend/parserUnroll.cpp

mihaibudiu · 2022-04-19T19:49:32Z

midend/parserUnroll.h

+    const IR::Expression& operator*() const { return *expr; }
+    const IR::Expression* operator->() const { return expr; }
+
+    // Implements comparisons so that StateVariables can be used as map keys.


If you use unordered_map you only need equality and hash.

Does p4c have some hash for IR::Expression? I used std::map. So, I turned back to operator<

midend/parserUnroll.h

mihaibudiu · 2022-04-19T20:01:05Z

midend/parserUnroll.cpp

-                    break;
-                else
-                    idx = i;
+            unsigned offset = 0;


can you explain what is happening here?

if it is a next operator then we need to make offset = 1, otherwise it should be 0. I did some refactoring there.

mihaibudiu · 2022-04-20T22:40:32Z

You do not seem to have addressed the comments made in my previous review.

VolodymyrPeschanenkoIntel · 2022-04-21T11:17:24Z

The spec does actually not mandate this behavior. The spec says that nextIndex should only be incremented if extracting directly to next. You can argue that this is a bug in the spec, but we should first take this change to the language design committee.

It looks very strange, because in the documentation of P4C we have:
hs.next: produces a reference to the element with index hs.nextIndex in the stack. May only be used in a parser. If the stack's nextIndex counter is greater than or equal to size, then evaluating this expression results in a transition to reject and sets the error to error.StackOutOfBounds. If hs is an l-value, then hs.next is also an l-value.
As I can see there are no restrictions for operator next usage. So, I think that we need to increment index of a header stack for next operator independently of a place where we use it. Am I right?

mihaibudiu · 2022-04-21T18:03:43Z

The spec does actually not mandate this behavior. The spec says that nextIndex should only be incremented if extracting directly to next. You can argue that this is a bug in the spec, but we should first take this change to the language design committee.

It looks very strange, because in the documentation of P4C we have: hs.next: produces a reference to the element with index hs.nextIndex in the stack. May only be used in a parser. If the stack's nextIndex counter is greater than or equal to size, then evaluating this expression results in a transition to reject and sets the error to error.StackOutOfBounds. If hs is an l-value, then hs.next is also an l-value. As I can see there are no restrictions for operator next usage. So, I think that we need to increment index of a header stack for next operator independently of a place where we use it. Am I right?

See https://p4.org/p4-spec/docs/P4-16-v1.2.2.html#sec-packet-extract-one, in particular the pseudo-code for extract. It says nothing about header_unions. That is arguably wrong, but it should be changed if it is wrong.

VolodymyrPeschanenkoIntel · 2022-04-22T10:37:07Z

The spec does actually not mandate this behavior. The spec says that nextIndex should only be incremented if extracting directly to next. You can argue that this is a bug in the spec, but we should first take this change to the language design committee.

It looks very strange, because in the documentation of P4C we have: hs.next: produces a reference to the element with index hs.nextIndex in the stack. May only be used in a parser. If the stack's nextIndex counter is greater than or equal to size, then evaluating this expression results in a transition to reject and sets the error to error.StackOutOfBounds. If hs is an l-value, then hs.next is also an l-value. As I can see there are no restrictions for operator next usage. So, I think that we need to increment index of a header stack for next operator independently of a place where we use it. Am I right?

See https://p4.org/p4-spec/docs/P4-16-v1.2.2.html#sec-packet-extract-one, in particular the pseudo-code for extract. It says nothing about header_unions. That is arguably wrong, but it should be changed if it is wrong.

I see. So, you said that "we should first take this change to the language design committee". How to do that?

jnfoster · 2022-04-22T13:19:37Z

For a change like this, which is sort of a bug fix, you can propose a PR on the p4-spec repository. Then we'll discuss at the next LDWG.

midend/parserUnroll.cpp

VolodymyrPeschanenkoIntel · 2022-06-21T08:45:16Z

@fruffy, @mbudiu-vmw Can we merge it?

VolodymyrPeschanenkoIntel added 2 commits April 8, 2022 12:38

fixing bug for sequential next usage

d8dce05

update for detection index of last

d487c87

add test

6f0bc28

fruffy approved these changes Apr 8, 2022

View reviewed changes

fruffy reviewed Apr 8, 2022

View reviewed changes

mihaibudiu mentioned this pull request Apr 8, 2022

Behavior of next index for extracting to header unions p4lang/p4-spec#1048

Closed

9 tasks

mihaibudiu requested changes Apr 8, 2022

View reviewed changes

add StackVariable class

d012e3f

VolodymyrPeschanenkoIntel requested review from mihaibudiu and fruffy April 19, 2022 12:07

fixing bug

49ef04b

mihaibudiu requested changes Apr 19, 2022

View reviewed changes

VolodymyrPeschanenkoIntel requested a review from mihaibudiu April 20, 2022 13:11

VolodymyrPeschanenkoIntel added 2 commits April 20, 2022 16:16

Merge branch 'main' into vp/puNextBug

9b9338a

fix failed test

22e2fd1

VolodymyrPeschanenkoIntel added 2 commits April 21, 2022 14:17

changes according review

0f9362a

add operator<

5fa6561

This was referenced Apr 22, 2022

Add header union information for extract operator VolodymyrPeschanenkoIntel/p4-spec#1

Closed

Add header union information for extract operator p4lang/p4-spec#1064

Merged

Merge branch 'main' into vp/puNextBug

b9bf250

VolodymyrPeschanenkoIntel added 3 commits June 9, 2022 15:47

update according review

147017f

fixing bug for stack variable storage

a9797f7

fixing bug

4814a02

mihaibudiu approved these changes Jun 14, 2022

View reviewed changes

midend/parserUnroll.cpp Outdated Show resolved Hide resolved

midend/parserUnroll.cpp Outdated Show resolved Hide resolved

changes according review

4569335

VolodymyrPeschanenkoIntel mentioned this pull request Jun 21, 2022

Enable loopsunrolling by default [recreated] #3142

Draft

mihaibudiu merged commit 5aa3b9a into p4lang:main Jun 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing parserUnroll bug for sequential next operator usage #3193

Fixing parserUnroll bug for sequential next operator usage #3193

VolodymyrPeschanenkoIntel commented Apr 8, 2022

fruffy commented Apr 8, 2022

fruffy left a comment

fruffy Apr 8, 2022

VolodymyrPeschanenkoIntel Apr 8, 2022

VolodymyrPeschanenkoIntel Apr 8, 2022

mihaibudiu Apr 8, 2022

mihaibudiu commented Apr 8, 2022

mihaibudiu commented Apr 8, 2022

mihaibudiu commented Apr 8, 2022

mihaibudiu Apr 8, 2022

VolodymyrPeschanenkoIntel commented Apr 19, 2022

mihaibudiu Apr 19, 2022

VolodymyrPeschanenkoIntel Apr 21, 2022

mihaibudiu Apr 19, 2022

VolodymyrPeschanenkoIntel Apr 21, 2022

mihaibudiu commented Apr 20, 2022

VolodymyrPeschanenkoIntel commented Apr 21, 2022

mihaibudiu commented Apr 21, 2022

VolodymyrPeschanenkoIntel commented Apr 22, 2022

jnfoster commented Apr 22, 2022

VolodymyrPeschanenkoIntel commented Jun 21, 2022

Fixing parserUnroll bug for sequential next operator usage #3193

Fixing parserUnroll bug for sequential next operator usage #3193

Conversation

VolodymyrPeschanenkoIntel commented Apr 8, 2022

fruffy commented Apr 8, 2022

fruffy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mihaibudiu commented Apr 8, 2022

mihaibudiu commented Apr 8, 2022

mihaibudiu commented Apr 8, 2022

Choose a reason for hiding this comment

VolodymyrPeschanenkoIntel commented Apr 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mihaibudiu commented Apr 20, 2022

VolodymyrPeschanenkoIntel commented Apr 21, 2022

mihaibudiu commented Apr 21, 2022

VolodymyrPeschanenkoIntel commented Apr 22, 2022

jnfoster commented Apr 22, 2022

VolodymyrPeschanenkoIntel commented Jun 21, 2022