Check that verify is only used in parsers.

[jnfoster]

Fix for issue #608

[jnfoster]

@mbudiu-vmw I'm not sure about this fix. Can you have a look?

In particular:

• Is validateParsedProgram the right place to do this check? I think so...

• Am I analyzing the IR correctly? That is, check for a MethodCallExpression whose method member is a PathExpression that is equal to ParserState::verify when converted to a cstring? Maybe there is a more elegant or complete way to detect this.

[mihaibudiu]

This will work with the current language definition, but it would be more robust to use MethodInstance to resolve the method. This check could probably be better placed in TypeInference::postorder(IR::MethodCallExpression* e). There is already a call to MethodInstance, one just needs to add a check for this particular built-in method.

[jnfoster]

Okay, I'll move it there. Thanks!

[jnfoster]

Whoops, actually I don't understand.

• The check is already in IR::MethodCallExpression. I extended the visitor to override that case. Are you suggesting moving it elsewhere? Or keeping it there?

• I do see that using MethodInstance could help streamline the analysis and make it more robust, but I don't see any uses in this pass. In particular, the typeMap and refMap are not available, and most of the resolve methods seem to require them. Which is the existing call you were referring to (maybe a link to a line number?)

[mihaibudiu]

I suggested TypeInference, the typeChecker.cpp file.

[jnfoster]

Ahh, I see. Got it.

[jnfoster]

Sorry, one more question: it's not obvious to me that verify is a built-in method according to methodInstance.cpp... The resolver seems to only construct a BuiltInMethod object for isValid/setValid/setInvalid and push_front/pop_front.

[mihaibudiu]

In that case this is actually an ExternFunction.

[jnfoster]

@mbudiu-vmw

1. I can add the following code to TypeInference::postorder(IR::MethodCallExpression)

        // Check that verify is only used in parsers.
        if (auto ef = mi->to<ExternFunction>()) {
            if (auto pe = ef->expr->method->to<IR::PathExpression>())
                if (pe->path->asString() == IR::ParserState::verify)
                    if (!findContext<IR::P4Parser>())
                        ::error("%1%: may only be invoked in parsers", ef->expr);
        }

However, the ExternFunction object is not helping me much -- unless I've missed something, I still have to go unpack the original MethodCallExpression to access the name. So I'm not sure this is better. Since validate is a built-in function, why not do the check in validateParsedProgram instead? It seems simpler and just as robust...

2. Should we also add checks for packet_in and packet_out to make sure they are only invoked in the parser/deparser? Or are those implemented further downstream in the back-end?

[mihaibudiu]

ef->method is a pointer to a Method object, which has a name.
Since it's an extern function you know that there is no object, and the name uniquely identifies the function.

One cannot create packet_in/out objects, so they only appear where the architecture allows them.
If the architecture is broken we can't do much in the compiler.

[sethfowler]

I haven't read through the recent comments in this issue carefully, but I did want to point out that frontends/p4/externInstance.h may be relevant here.

[jnfoster]

@cc10512 I'm going with Mihai's proposal. Perhaps this does belong earlier, but we are getting some mileage out of the MethodInstance class so it is definitely more robust. It can always be revisited later.