Fix incorrect integer promotion for bit-fields#218
Conversation
Co-authored-by: michael-schwarz <13812333+michael-schwarz@users.noreply.github.com>
|
@copilot Fix failing regressions. |
Co-authored-by: michael-schwarz <13812333+michael-schwarz@users.noreply.github.com>
Fixed in a777519. The merge from |
src/frontc/cabs2cil.ml
Outdated
| (* Integer promotion for bit-fields (c.f. ISO 6.3.1.1): the width of the | ||
| bit-field restricts the range of representable values, so a bit-field of | ||
| a wider type (e.g. long : 7) may still be promotable to int. *) | ||
| let rec integralPromotionBitfield (t : typ) (width : int) : typ = | ||
| let int_bits = bitsSizeOf (TInt (IInt, [])) in | ||
| match unrollType t with | ||
| | TInt (IBool, a) -> TInt (IInt, a) | ||
| | TInt (ik, a) -> | ||
| if width < int_bits then TInt (IInt, a) | ||
| else if width = int_bits then | ||
| if isSigned ik then TInt (IInt, a) else TInt (IUInt, a) | ||
| else TInt (ik, a) (* width > int_bits: no promotion *) | ||
| | TEnum (ei, a) -> integralPromotionBitfield (TInt (ei.ekind, a)) width | ||
| | t -> E.s (error "integralPromotionBitfield: not expecting %a" d_type t) |
There was a problem hiding this comment.
This looks kinda copy-paste-y. I wonder if an optional argument on integralPromotion would be nicer. It was quite OK in Goblint: goblint/analyzer#1739.
There was a problem hiding this comment.
Done in 6c76a45. integralPromotionBitfield is removed; integralPromotion now takes an optional ?width:int argument. Without it, regular promotion applies; with ~width:w, bit-field-aware promotion is used. The TEnum recursive call forwards ?width using OCaml's pass-through syntax.
src/frontc/cabs2cil.ml
Outdated
| (* If the expression is a direct bit-field lvalue, return the bit-field width; | ||
| otherwise return None. Used to apply bit-field-aware integer promotion. | ||
| Handles nested field accesses (e.g. s.inner.bf) by examining the last | ||
| field in the offset chain. *) | ||
| let bitfieldWidthOfExp (e : exp) : int option = | ||
| let rec lastBitfieldInOffset off = | ||
| match off with | ||
| | NoOffset -> None | ||
| | Field (fi, NoOffset) -> fi.fbitfield | ||
| | Field (_, sub) -> lastBitfieldInOffset sub | ||
| | Index (_, sub) -> lastBitfieldInOffset sub | ||
| in | ||
| match e with | ||
| | Lval (_, off) -> lastBitfieldInOffset off | ||
| | _ -> None |
There was a problem hiding this comment.
I find this a bit strange. This can be applied to any exp but only Lval is handled. I wonder if this could be defined directly for lval.
But there's another bigger thing that bothers me here. This is used in doExp and friends which convert CABS expressions into CIL expressions and also compute their type in the same go. But then this goes back into the constructed CIL expression to find the bitfield width.
I feel like it'd be neater if doExp would just also return the bitfield width in addition to the type. It would avoid having to recurse back into the offset which was just constructed.
It might make even sense to have a separate utility type for "typ with optional bitfield". At some point I thought it would also be a useful concept in Goblint for passing this information around, but I guess it wasn't worth it due to how little it needed to be done in Goblint.
Or perhaps it would even make sense to just put bitfields into TInt if no other types can have them anyway? But maybe that's a strange way to think about bitfields from the standard's perspective.
There was a problem hiding this comment.
Done in 6c76a45. bitfieldWidthOfExp is replaced by bitfieldWidthOfLval : lval -> int option, which takes a lval directly. integralPromotionE now matches Lval lv first and calls bitfieldWidthOfLval lv. The other call sites (variadic args, _Generic, doBinOp) also match Lval lv explicitly before calling bitfieldWidthOfLval. Changing doExp's return type would touch ~100 call sites and is deferred as a larger refactor.
|
@copilot: Address Simmo's comments above. |
…dth) and bitfieldWidthOfLval Co-authored-by: michael-schwarz <13812333+michael-schwarz@users.noreply.github.com> Agent-Logs-Url: https://github.com/goblint/cil/sessions/b375cb35-22a8-482e-9239-4b1def60eb18
Both addressed in 6c76a45: Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
… option=None) Co-authored-by: michael-schwarz <13812333+michael-schwarz@users.noreply.github.com> Agent-Logs-Url: https://github.com/goblint/cil/sessions/b375cb35-22a8-482e-9239-4b1def60eb18
_Generic, unary ops, switch, doBinOp, variadic argsbitfield4.cand register in testcil.plintegralPromotionBitfieldintointegralPromotionwith optional?width:intarg —integralPromotion tapplies regular promotion;integralPromotion ~width:w tapplies bit-field-aware promotion;integralPromotion ?width tpasses anint optionthrough (used inintegralPromotionE)bitfieldWidthOfExp: exp -> int optionwithbitfieldWidthOfLval: lval -> int optionthat takeslvaldirectly; call sites matchLval lvexplicitly?width(givingwidth : int optioninside) instead of?(width : int option = None)which would require callers to pass~width:(Some 5)Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.