Fixup/cleanup IR::Type::width_bits() #4858
Conversation
fruffy
added
the
core
Looks like the code is calling type->width_bits() unconditionally, even thought the value it gets back only matters when type happens to be Type::Bits. Swapping the order of the clauses is the |
There was a problem hiding this comment.
P4Testgen fails because it calls
width_bitsonType_String. Why, I do not quite remember but it should be a benign issues when serializing a test.Looks like the code is calling type->width_bits() unconditionally, even thought the value it gets back only matters when type happens to be Type::Bits. Swapping the order of the clauses is the
ifavoids the problem (and is more efficient too)
Makes sense, this was part of a cache implementation based on bit-width. I think there is also code like this in lib/expression.cpp.
kfcripps
added
the
breaking-change
| @@ -321,7 +321,7 @@ class TypeNameExpression : Expression { | |||
| dbprint{ out << typeName; } | |||
| toString { return typeName->toString(); } | |||
| validate { BUG_CHECK(typeName->is<Type_Name>() || typeName->is<Type_Specialized>(), | |||
| "%1 unexpected type in TypeNameExpression", typeName); } | |||
| "%1% unexpected type in TypeNameExpression", typeName); } | |||
There was a problem hiding this comment.
the sooner we migrate out of boost::format, the better. The format strings should be checked statically at compile time as much as possible :)
There was a problem hiding this comment.
So we should use C printf instead so we can use __attribute__((__format__ to get compile time checking? I'm not aware of any way to get the C++ formatting stuff (either boost or std::fmt) to check things at compile time.
There was a problem hiding this comment.
abseil does this. fmtlib does as well. Actually, the places where we do use abseil formatting routines already perform compile-time checking (e.g. SourceCodeBuilder::appendFormat) and it takes some jumps to disable this checking (e.g. in error_reporter)
ChrisDodd
force-pushed
the
cdodd-misc
branch
2 times, most recently
from
c33e8bf to
ec73f2f
Compare
ChrisDodd
force-pushed
the
cdodd-misc
branch
2 times, most recently
from
fae4103 to
1e75aec
Compare
- this method is used by backends to get the size of a type, and should only be used for Types for which the concept of `size in bits' makes sense Signed-off-by: Chris Dodd <[email protected]>
| @@ -268,6 +269,7 @@ class Type_InfInt : Type, ITypeVar { | |||
| return true; /* ignore declid */ | |||
| } | |||
| const Type* getP4Type() const override { return this; } | |||
| int width_bits() const override { return 0; } | |||
There was a problem hiding this comment.
Hmm, is this a good idea? with_bits fails for some types which don't have defined width, yet here it returns 0 as a "don't know" value (even though 0 can also be a perfectly valid with of bit<0>).
There was a problem hiding this comment.
Yes, it would be better to remove this, but there are many places in that seem to depend on this.
There was a problem hiding this comment.
Sadly, there are more cases that someone could have relied on (whether deliberately or as a bug, e.g. void and unknown). Ultimately I think the BUG makes sense though (and to me it would make sense for int too).
There was a problem hiding this comment.
A common case that hits this is code that is converting constants to strings of some kind (eg, json or other output). It generally just calls width_bits() on the constant type and if it returns 0 ouputs a constant with no specific size (if >0, it outputs some size indications). This happens to be what you want, but relies on InfInt::width_bits() returning 0.
This method is used by backends to get the size of a type, and should only be used for Types for which the concept of `size in bits' makes sense. Having it return 0 for types that it should not be called for adds to the confusion and possible bugs when it gets called when it shouldn't. It is suprising to me that having it return 0 for stacks and serializable enums hasn't caused problems before -- bugs waiting to happen.
The existence of this method goes back to the very earliest days of the compiler -- it can be argued that this does not belong here, and it really should be target specific, but the fact that almost all targets will have the same sizes for many things, plus the inability to do multiple dispatch in C++ is what caused it to be implements as a IR::Type virtual method.