Testgen: fix varbit handling

[vhavel]

Instead of enriching the program IR by a custom varbit type which remembers extracted size, assign bit<extracted_size> directly to varbit members.

The original approach with custom Extracted_Varbit type is problematic: the type inherits from bit type where X is the maximum size of corresponding varbit type, however the width_bits method is also overridden to return the actually extracted varbit size.

[vhavel]

@fruffy I decided to simplify this PR - it still solves the problem I'm facing, but without complex refactoring of varbit handling in testgen.

The problem with Extracted_Varbits<X> is that is inherits from bit<X>, so any code which is oblivious to this type can mistakenly assume the type/value is actually bit<X>. Surprisingly this doesn't occur often, but in our case it happens because canonicalize function from P4 typechecking returns bit<X> instead of the custom type, which confuses a midend pass in our target. That leads to storing bit<X> in the symbolic map for an invalid header field, which should not break anything, however parser extracts detected Extracted_Varbits based on the value stored in the symbolic map, so the incorrectly typed value prevented correct detection. This PR doesn't address the general problem of varbit handling in testgen, but rather fixes just the problem of detecting Extracted_Varbits values by looking at the type of the member instead.

[fruffy]

Yes, inheriting from Type_Bits was intentional so that we can actually use all the compiler optimizations we apply without having to copy and extend them. We made some modifications to passes to preserve the type, too.

Unfortunately, I do not know a better way to solve this.

Maybe there is a way to preserve the type in canonicalize?

[vhavel]

Maybe there is a way to preserve the type in canonicalize?

In theory yes, type canonicalization can be customized, however the problematic pass is a re-used pass from bf-p4c compiler, i.e. a codebase which is completely Extracted_Varbits-oblivious. I'm surprised similar problems do not arise elsewhere, I consider Extracted_Varbits does break the substitution principle a bit (is it really substitutable by Type_Bits?).

Unfortunately, I do not know a better way to solve this.

In my opinion, more clean solution would be to preserve the varbit type in the IR, and storing bit values (and changing testgen internals to handle different type of key and value in the symbolic map). But it's relatively large change, and I'm still unsure if it's worth given that the current approach seems to work well in reality.

Anyway, I think the change in this PR is still step in right direction, because the type in IR (keys in symbolic map) should determine how the member should be handled, not the type of the currently stored value.