Skip to content

Conversation

@vlstill
Copy link
Member

@vlstill vlstill commented May 5, 2025

Fixes a crash when the compiler tries to create int<0> constant:

Compiler bug triggered, please report this error:
exception type: boost::wrapexcept<std::out_of_range>
error: Can not shift by a negative value.

Also fixes the warning category of bit<N> overflow (which was mismatch and now is overflow).

@vlstill vlstill force-pushed the vstill/fix-bit-0-overflow branch 2 times, most recently from c2956c5 to 08069e4 Compare May 5, 2025 14:36
@vlstill vlstill added tofino Topics related to the Tofino switch and back end. core Topics concerning the core segments of the compiler (frontend, midend, parser) labels May 5, 2025
@vlstill vlstill force-pushed the vstill/fix-bit-0-overflow branch from 5038bd6 to 91ed8be Compare May 6, 2025 12:01
@vlstill vlstill marked this pull request as ready for review May 6, 2025 17:23
@vlstill vlstill requested review from asl and qobilidop May 6, 2025 17:23
@vlstill vlstill removed the tofino Topics related to the Tofino switch and back end. label May 7, 2025
@vlstill vlstill requested review from ChrisDodd and fruffy May 7, 2025 07:36
@asl
Copy link
Contributor

asl commented May 7, 2025

How can such values appear?

@vlstill
Copy link
Member Author

vlstill commented May 14, 2025

How can such values appear?

Altera backend certainly hits it. It might not be directly from the code, but I think even a manual creation of such constant must not trigger exception in boost.

@vlstill vlstill force-pushed the vstill/fix-bit-0-overflow branch from e06daee to 59be003 Compare May 14, 2025 09:34
@asl
Copy link
Contributor

asl commented May 14, 2025

Altera backend certainly hits it. It might not be directly from the code, but I think even a manual creation of such constant must not trigger exception in boost.

I am actually thinking, if creation of such types is a result of some underlying issue? Shouldn't int<0> be just disallowed?

@jafingerhut
Copy link
Contributor

jafingerhut commented May 14, 2025

Altera backend certainly hits it. It might not be directly from the code, but I think even a manual creation of such constant must not trigger exception in boost.

I am actually thinking, if creation of such types is a result of some underlying issue? Shouldn't int<0> be just disallowed?

Section 7.1.6.2 "Unsigned integers (bit-strings)" of the P4 language spec includes this sentence: "Bitstrings with width 0 are allowed; they have no actual bits, and can only have the value 0." This was added to the language spec in Apr 2021 from some git log searches.

For type int<W> the spec is not quite as explicit about W=0, but it does say this: "W must be an expression that evaluates to a local compile-time known (see Section [Section 18.1]) value that is a non-negative integer." That allows 0.

@ChrisDodd
Copy link
Contributor

ChrisDodd commented May 15, 2025

I am actually thinking, if creation of such types is a result of some underlying issue? Shouldn't int<0> be just disallowed?

bit<0> was added to allow code like

header foo {
    bit<(DEFINED_CONST)> data;
    bit<(-(DEFINED_CONST) & 7)> pad;  // pad out to byte boundary
};

where DEFINED_CONST is defined by a macro or -D argument

int<0> was added for consistency

@asl
Copy link
Contributor

asl commented May 15, 2025

int<0> was added for consistency

Then probably the meaning should be clarified? Should the spec explicitly say that bit<0> and int<0> behave similarly?

if (tb->isSigned) {
if (width == 0) {
if (value != 0 && !noWarning) {
P4::warning(ErrorType::WARN_OVERFLOW,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should use warning instead of P4::warning for consistency with below.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Topics concerning the core segments of the compiler (frontend, midend, parser)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants