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

Narrowing with bottom does not produce bottom #1540

Closed
Red-Panda64 opened this issue Jul 9, 2024 · 7 comments · Fixed by #1543
Closed

Narrowing with bottom does not produce bottom #1540

Red-Panda64 opened this issue Jul 9, 2024 · 7 comments · Fixed by #1543

Comments

@Red-Panda64
Copy link
Contributor

Below is a slightly reformatted (removed indentation and added line breaks to make this more readable) traced message which shows an
a value being narrowed with bot and remaining unchanged.

%%% narrow: narrow HConsed lifted PathSensitive (ProjectiveSet (MCP.D * map)):PathSensitive (ProjectiveSet (MCP.D * map)):{(MCP.D:[expRelation:(),
mallocWrapper:(wrapper call:Unknown node, unique calls:{}),
base:({
Parameter {
i ->   (Unknown int([-31,31]),[0,2147483647])
}
}, {}, {}, (P:{}, Protected Changes:{
})),
threadid:(wrapper call:varinfo * node * chain option:(varinfo:f, node:node 21 "pthread_create((pthread_t * __restrict  )(& id), (pthread_attr_t const   * __restrict  )((void *)0), & f, (void * __restrict  )((void *)0));", chain option:no index), Thread:[main, f@tests/regression/95-dividedsides/19-dead-side-effect3.c:26:3-26:36], created:(current function:bot, callees:bot)),
threadflag:Multithreaded (other),
threadreturn:False,
escape:{},
mutexEvents:(),
access:(),
mutex:(lockset:{}, multiplicity:{}),
race:(),
mhp:(),
assert:()], map:{})}

with bot to

HConsed lifted PathSensitive (ProjectiveSet (MCP.D * map)):PathSensitive (ProjectiveSet (MCP.D * map)):{(MCP.D:[expRelation:(),
mallocWrapper:(wrapper call:Unknown node, unique calls:{}),
base:({
Parameter {
i ->   (Unknown int([-31,31]),[0,2147483647])
}
}, {}, {}, (P:{}, Protected Changes:{
})),
threadid:(wrapper call:varinfo * node * chain option:(varinfo:f, node:node 21 "pthread_create((pthread_t * __restrict  )(& id), (pthread_attr_t const   * __restrict  )((void *)0), & f, (void * __restrict  )((void *)0));", chain option:no index), Thread:[main, f@tests/regression/95-dividedsides/19-dead-side-effect3.c:26:3-26:36], created:(current function:bot, callees:bot)),
threadflag:Multithreaded (other),
threadreturn:False,
escape:{},
mutexEvents:(),
access:(),
mutex:(lockset:{}, multiplicity:{}),
race:(),
mhp:(),
assert:()], map:{})}

There are some domains (e.g. DefExc) that do not narrow to bottom. The interval should be narrowed to bottom, however. It seems this is prevented by some lifter further up.

@Red-Panda64
Copy link
Contributor Author

Red-Panda64 commented Jul 10, 2024

I have discovered this snippet in src/domain/lattice.ml:

  let widen x y =
    match (x,y) with
    | (`Lifted x, `Lifted y) -> `Lifted (Base.widen x y)
    | _ -> y

  let narrow x y =
    match (x,y) with
    | (`Lifted x, `Lifted y) -> `Lifted (Base.narrow x y)
    | _ -> x

These are part of the LiftConf module, which also has constructors `Top and `Bot. Not only does this mean, that narrowing with bottom is a nop instead of returning bottom, it also means that widen (`Top, `Lifted y) returns `Lifted y, which is less than `Top.

@michael-schwarz
Copy link
Member

Oops!

@Red-Panda64
Copy link
Contributor Author

Same applies to LiftPO, Lift2Conf, LiftBot and LiftTop. Maybe I am missing something here, but that seems wrong to me.

@michael-schwarz
Copy link
Member

The correctness argument is that our solvers ensure that for widen the second argument is larger and for narrow the second argument is smaller. But I'm not sure whether all lifters maintain this, so making it more explicit may be a good idea.

@sim642
Copy link
Member

sim642 commented Jul 10, 2024

For widen that is documented and guaranteed last time I checked.

For narrow, I'm hearing that assumption for the first time. TD3 doesn't guarantee this (#490 (comment)) and this was considered expected when we discussed it (to remove the assert). Our properties for narrow have matched the textbook definitions:

let narrow_meet = make ~name:"narrow meet" (pair arb arb) (fun (a, b) -> D.leq (D.meet a b) (D.narrow a b))
let narrow_fst = make ~name:"narrow fst" (pair arb arb) (fun (a, b) -> D.leq (D.narrow a b) a)
.

@Red-Panda64
Copy link
Contributor Author

I see. In terms of correctness, the narrowing implementation is not an issue. The widening is an oversight on my part. Still, as discussed with @michael-schwarz, it would be nice for narrowing with bottom to produce bottom for a mechanism we are working on. Is it acceptable to change the narrowing operators to produce bottom whenever possible? If not, it is not a big issue, as I already have a work-around.

@sim642
Copy link
Member

sim642 commented Jul 10, 2024

Is it acceptable to change the narrowing operators to produce bottom whenever possible?

I think it makes sense to change the various Lift/Flat/... lattice functors to do that. It's consistent with the two properties.

@sim642 sim642 added this to the v2.4.0 milestone Jul 16, 2024
sim642 added a commit to sim642/opam-repository that referenced this issue Aug 2, 2024
CHANGES:

* Remove unmaintained analyses: spec, file (goblint/analyzer#1281).
* Add linear two-variable equalities analysis (goblint/analyzer#1297, goblint/analyzer#1412, goblint/analyzer#1466).
* Add callstring, loopfree callstring and context gas analyses (goblint/analyzer#1038, goblint/analyzer#1340, goblint/analyzer#1379, goblint/analyzer#1427, goblint/analyzer#1439).
* Add non-relational thread-modular value analyses with thread IDs (goblint/analyzer#1366, goblint/analyzer#1398, goblint/analyzer#1399).
* Add NULL byte array domain (goblint/analyzer#1076).
* Fix spurious overflow warnings from internal evaluations (goblint/analyzer#1406, goblint/analyzer#1411, goblint/analyzer#1511).
* Refactor non-definite mutex handling to fix unsoundness (goblint/analyzer#1430, goblint/analyzer#1500, goblint/analyzer#1503, goblint/analyzer#1409).
* Fix non-relational thread-modular value analysis unsoundness with ambiguous points-to sets (goblint/analyzer#1457, goblint/analyzer#1458).
* Fix mutex type analysis unsoundness and enable it by default (goblint/analyzer#1414, goblint/analyzer#1416, goblint/analyzer#1510).
* Add points-to set refinement on mutex path splitting (goblint/analyzer#1287, goblint/analyzer#1343, goblint/analyzer#1374, goblint/analyzer#1396, goblint/analyzer#1407).
* Improve narrowing operators (goblint/analyzer#1502, goblint/analyzer#1540, goblint/analyzer#1543).
* Extract automatic configuration tuning for soundness (goblint/analyzer#1369).
* Fix many locations in witnesses (goblint/analyzer#1355, goblint/analyzer#1372, goblint/analyzer#1400, goblint/analyzer#1403).
* Improve output readability (goblint/analyzer#1294, goblint/analyzer#1312, goblint/analyzer#1405, goblint/analyzer#1497).
* Refactor logging (goblint/analyzer#1117).
* Modernize all library function specifications (goblint/analyzer#1029, goblint/analyzer#688, goblint/analyzer#1174, goblint/analyzer#1289, goblint/analyzer#1447, goblint/analyzer#1487).
* Remove OCaml 4.10, 4.11, 4.12 and 4.13 support (goblint/analyzer#1448).
avsm pushed a commit to avsm/opam-repository that referenced this issue Sep 5, 2024
CHANGES:

* Remove unmaintained analyses: spec, file (goblint/analyzer#1281).
* Add linear two-variable equalities analysis (goblint/analyzer#1297, goblint/analyzer#1412, goblint/analyzer#1466).
* Add callstring, loopfree callstring and context gas analyses (goblint/analyzer#1038, goblint/analyzer#1340, goblint/analyzer#1379, goblint/analyzer#1427, goblint/analyzer#1439).
* Add non-relational thread-modular value analyses with thread IDs (goblint/analyzer#1366, goblint/analyzer#1398, goblint/analyzer#1399).
* Add NULL byte array domain (goblint/analyzer#1076).
* Fix spurious overflow warnings from internal evaluations (goblint/analyzer#1406, goblint/analyzer#1411, goblint/analyzer#1511).
* Refactor non-definite mutex handling to fix unsoundness (goblint/analyzer#1430, goblint/analyzer#1500, goblint/analyzer#1503, goblint/analyzer#1409).
* Fix non-relational thread-modular value analysis unsoundness with ambiguous points-to sets (goblint/analyzer#1457, goblint/analyzer#1458).
* Fix mutex type analysis unsoundness and enable it by default (goblint/analyzer#1414, goblint/analyzer#1416, goblint/analyzer#1510).
* Add points-to set refinement on mutex path splitting (goblint/analyzer#1287, goblint/analyzer#1343, goblint/analyzer#1374, goblint/analyzer#1396, goblint/analyzer#1407).
* Improve narrowing operators (goblint/analyzer#1502, goblint/analyzer#1540, goblint/analyzer#1543).
* Extract automatic configuration tuning for soundness (goblint/analyzer#1369).
* Fix many locations in witnesses (goblint/analyzer#1355, goblint/analyzer#1372, goblint/analyzer#1400, goblint/analyzer#1403).
* Improve output readability (goblint/analyzer#1294, goblint/analyzer#1312, goblint/analyzer#1405, goblint/analyzer#1497).
* Refactor logging (goblint/analyzer#1117).
* Modernize all library function specifications (goblint/analyzer#1029, goblint/analyzer#688, goblint/analyzer#1174, goblint/analyzer#1289, goblint/analyzer#1447, goblint/analyzer#1487).
* Remove OCaml 4.10, 4.11, 4.12 and 4.13 support (goblint/analyzer#1448).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants