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

[bug/enhancement] boolean fields #272

Closed
smolkaj opened this issue Feb 1, 2017 · 5 comments
Closed

[bug/enhancement] boolean fields #272

smolkaj opened this issue Feb 1, 2017 · 5 comments

Comments

@smolkaj
Copy link
Member

smolkaj commented Feb 1, 2017

Currently, the compiler does not accept boolean metadata, and raises errors such as this one:

terminate called after throwing an instance of 'Util::CompilerBug'
  what():  COMPILER BUG: ../frontends/p4/typeChecking/typeChecker.h:100
./includes/meta.p4(90): Field bool urpf_hit of header l3_metadata_t cannot have type bool
    bool urpf_hit;
         ^^^^^^^^

Aborted (core dumped)

This seems like an arbitrary (and in practice, very annoying) restriction. Are there deeper reasons for this or is it just something that hasn't been implemented yet?
Boolean metadata is extremely common in switch.p4. It would be convenient to use bool rather than bit<1> for this purpose.

@smolkaj smolkaj changed the title [bug/enhancement] [bug/enhancement] boolean metadata Feb 1, 2017
@mihaibudiu
Copy link
Contributor

There is no "metadata" in P4-16 really; there are just structs, and they can have boolean fields. The spec forbids boolean fields in headers, and the error message indicates that this is a header. However, the error is a BUG, so it looks like this header was somehow synthesized by the compiler internally. Can you post a some code to reproduce this error, please?

@mihaibudiu
Copy link
Contributor

This is actually easy to reproduce; it happens for a header with a bool field.
This is a bug; the compiler should just emit an error.
However, the question is whether we should allow boolean fields in headers. Most protocol specifications do not use booleans; headers are always expressed in bitstrings. So this issue should probably be filed first with the P4-16 spec.

mihaibudiu pushed a commit to mihaibudiu/p4c-clone that referenced this issue Feb 1, 2017
mihaibudiu pushed a commit to mihaibudiu/p4c-clone that referenced this issue Feb 1, 2017
@smolkaj
Copy link
Member Author

smolkaj commented Feb 1, 2017

@mbudiu-vmw: I opened an issue in p4-spec as you suggested, see p4lang/p4-spec#123. I cannot think of any good reasons to disallow booleans in header fields, but let's discuss there.

ChrisDodd pushed a commit that referenced this issue Feb 1, 2017
* Fix for issue #272

* Small documentation fix
@smolkaj smolkaj changed the title [bug/enhancement] boolean metadata [bug/enhancement] boolean header fields Feb 2, 2017
@smolkaj smolkaj changed the title [bug/enhancement] boolean header fields [bug/enhancement] boolean fields Feb 2, 2017
@smolkaj
Copy link
Member Author

smolkaj commented Feb 2, 2017

@mbudiu-vmw: Actually, it seems that structs cannot have boolean fields. I get the error

terminate called after throwing an instance of 'Util::CompilerBug'
  what():  COMPILER BUG: ../backends/bmv2/jsonconverter.cpp:2122
switch.2.p4(6): bool: unexpected type for struct some_meta_t {
  bool flag; }.flag
struct some_meta_t { bool flag; }
       ^^^^^^^^^^^
switch.2.p4(6)
struct some_meta_t { bool flag; }

when trying to compile

#include <core.p4>
#include <v1model.p4>

typedef standard_metadata_t std_meta_t;

struct some_meta_t { bool flag; }

struct H { }

struct M { some_meta_t some_meta; }

control DeparserI(packet_out packet, in H hdr) {
    apply { }
}

parser ParserI(packet_in pk, out H hdr, inout M meta, inout std_meta_t std_meta) {
    state start { transition accept; }
}

control VerifyChecksumI(in H hdr, inout M meta) {
    apply { }
}

control ComputeChecksumI(inout H hdr, inout M meta) {
    apply { }
}

control IngressI(inout H hdr, inout M meta, inout std_meta_t std_meta) {
    apply { }
}

control EgressI(inout H hdr, inout M meta, inout std_meta_t std_meta) {
    apply { }
}

V1Switch(ParserI(), VerifyChecksumI(), IngressI(), EgressI(), ComputeChecksumI(), DeparserI()) main;

mihaibudiu pushed a commit to mihaibudiu/p4c-clone that referenced this issue Feb 2, 2017
ChrisDodd pushed a commit that referenced this issue Feb 2, 2017
@smolkaj
Copy link
Member Author

smolkaj commented Feb 3, 2017

Great, this is fixed 👍

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

No branches or pull requests

2 participants