[NFC] Split TypeInference

[asl]

typeInference.cpp is a huge file spanning over 182 Kb (!). This PR splits into a bit more manageable chunks:

• declarations
• types
• statements
• expressions

Some very small cleanup while there. Overall, there are no functionality changes.

[ChrisDodd]

After this change, whenever I run ctest locally, I see 26 tests consistently fail -- all are _error tests (so expect specific errors), but instead fail with

raw_hash_set.h:3154: void absl::lts_20240116::container_internal::raw_hash_set<Policy, Hash, Eq, Alloc>::emplace_at(size_t, Args&& ...) [with Args = {const IR::Type_Var*&}; Policy = absl::lts_20240116::container_internal::FlatHashSetPolicy<const IR::ITypeVar*>; Hash = Util::Hash; Eq = absl::lts_20240116::container_internal::HashEq<const IR::ITypeVar*, void>::Eq; Alloc = std::allocator<const IR::ITypeVar*>; size_t = long unsigned int]: Assertion `PolicyTraits::apply(FindElement{*this}, *iterator_at(i)) == iterator_at(i) && "constructed value does not match the lookup key"' failed.

Any idea what causes this? Seems like something internal to absl.

[asl]

After this change, whenever I run ctest locally, I see 26 tests consistently fail -- all are _error tests (so expect specific errors), but instead fail with

raw_hash_set.h:3154: void absl::lts_20240116::container_internal::raw_hash_set<Policy, Hash, Eq, Alloc>::emplace_at(size_t, Args&& ...) [with Args = {const IR::Type_Var*&}; Policy = absl::lts_20240116::container_internal::FlatHashSetPolicy<const IR::ITypeVar*>; Hash = Util::Hash; Eq = absl::lts_20240116::container_internal::HashEq<const IR::ITypeVar*, void>::Eq; Alloc = std::allocator<const IR::ITypeVar*>; size_t = long unsigned int]: Assertion `PolicyTraits::apply(FindElement{*this}, *iterator_at(i)) == iterator_at(i) && "constructed value does not match the lookup key"' failed.

Any idea what causes this? Seems like something internal to absl.

Interesting... What are these tests? And can you try to backtrace to see where emplace is called?

[ChrisDodd]

The failing tests (from LastTestsFailed.log) are:

821:dpdk/testdata/p4_16_dpdk_errors/pna-example-ipsec-err3.p4
842:dpdk/testdata/p4_16_pna_errors/pna-example-mirror-packet-error2.p4
843:dpdk/testdata/p4_16_pna_errors/pna-example-mirror-packet-error3.p4
2280:err/testdata/p4_16_errors/constructor3_e.p4
2305:err/testdata/p4_16_errors/factory-err2.p4
2306:err/testdata/p4_16_errors/function1_e.p4
2307:err/testdata/p4_16_errors/function_e.p4
2327:err/testdata/p4_16_errors/issue1006-1.p4
2347:err/testdata/p4_16_errors/issue2021.p4
2349:err/testdata/p4_16_errors/issue2036-1.p4
2350:err/testdata/p4_16_errors/issue2036-2.p4
2351:err/testdata/p4_16_errors/issue2036.p4
2354:err/testdata/p4_16_errors/issue2230-1-bmv2.p4
2356:err/testdata/p4_16_errors/issue2260-1.p4
2378:err/testdata/p4_16_errors/issue3045-1.p4
2379:err/testdata/p4_16_errors/issue3045-2.p4
2380:err/testdata/p4_16_errors/issue3045.p4
2391:err/testdata/p4_16_errors/issue3246.p4
2413:err/testdata/p4_16_errors/issue345.p4
2458:err/testdata/p4_16_errors/issue774-1.p4
2460:err/testdata/p4_16_errors/issue803-1.p4
2468:err/testdata/p4_16_errors/list-error.p4
2475:err/testdata/p4_16_errors/module_e.p4
2478:err/testdata/p4_16_errors/named-fail1.p4
2492:err/testdata/p4_16_errors/push_nonconstant.p4
2543:err/testdata/p4_16_errors/typecheck_e.p4

partial backtrace from gdb with one of the tests:

#4  0x00007ffff7a66e96 in __assert_fail () from /lib/x86_64-linux-gnu/libc.so.6
#5  0x00005555588346aa in absl::lts_20240116::container_internal::raw_hash_set<absl::lts_20240116::container_internal::FlatHashSetPolicy<IR::ITypeVar const*>, Util::Hash, absl::lts_20240116::container_internal::HashEq<IR::ITypeVar const*, void>::Eq, std::allocator<IR::ITypeVar const*> >::emplace_at<IR::Type_Var const*&> (this=0x7fffffffbe68, i=0) at /home/cdodd/repos/p4org-p4c-alt/build/_deps/abseil-src/absl/container/internal/raw_hash_set.h:3154
#6  0x0000555558833c37 in absl::lts_20240116::container_internal::raw_hash_set<absl::lts_20240116::container_internal::FlatHashSetPolicy<IR::ITypeVar const*>, Util::Hash, absl::lts_20240116::container_internal::HashEq<IR::ITypeVar const*, void>::Eq, std::allocator<IR::ITypeVar const*> >::EmplaceDecomposable::operator()<IR::Type_Var const*, IR::Type_Var const*&> (this=0x7fffffffb5f0, key=@0x7fffffffb610: 0x7ffff77bd300)
    at /home/cdodd/repos/p4org-p4c-alt/build/_deps/abseil-src/absl/container/internal/raw_hash_set.h:2825
#7  0x00005555588331f5 in absl::lts_20240116::container_internal::DecomposeValue<absl::lts_20240116::container_internal::raw_hash_set<absl::lts_20240116::container_internal::FlatHashSetPolicy<IR::ITypeVar const*>, Util::Hash, absl::lts_20240116::container_internal::HashEq<IR::ITypeVar const*, void>::Eq, std::allocator<IR::ITypeVar const*> >::EmplaceDecomposable, IR::Type_Var const*&> (f=..., arg=@0x7fffffffb610: 0x7ffff77bd300)
    at /home/cdodd/repos/p4org-p4c-alt/build/_deps/abseil-src/absl/container/internal/container_memory.h:216
#8  0x000055555883291c in absl::lts_20240116::container_internal::FlatHashSetPolicy<IR::ITypeVar const*>::apply<absl::lts_20240116::container_internal::raw_hash_set<absl::lts_20240116::container_internal::FlatHashSetPolicy<IR::ITypeVar const*>, Util::Hash, absl::lts_20240116::container_internal::HashEq<IR::ITypeVar const*, void>::Eq, std::allocator<IR::ITypeVar const*> >::EmplaceDecomposable, IR::Type_Var const*&> (f=...)
    at /home/cdodd/repos/p4org-p4c-alt/build/_deps/abseil-src/absl/container/flat_hash_set.h:487
#9  0x0000555558831a04 in absl::lts_20240116::container_internal::hash_policy_traits<absl::lts_20240116::container_internal::FlatHashSetPolicy<IR::ITypeVar const*>, void>::apply<absl::lts_20240116::container_internal::raw_hash_set<absl::lts_20240116::container_internal::FlatHashSetPolicy<IR::ITypeVar const*>, Util::Hash, absl::lts_20240116::container_internal::HashEq<IR::ITypeVar const*, void>::Eq, std::allocator<IR::ITypeVar const*> >::EmplaceDecomposable, IR::Type_Var const*&, absl::lts_20240116::container_internal::FlatHashSetPolicy<IR::ITypeVar const*> > (f=...)
    at /home/cdodd/repos/p4org-p4c-alt/build/_deps/abseil-src/absl/container/internal/hash_policy_traits.h:134
#10 0x0000555558830921 in absl::lts_20240116::container_internal::raw_hash_set<absl::lts_20240116::container_internal::FlatHashSetPolicy<IR::ITypeVar const*>, Util::Hash, absl::lts_20240116::container_internal::HashEq<IR::ITypeVar const*, void>::Eq, std::allocator<IR::ITypeVar const*> >::emplace<IR::Type_Var const*&, 0> (this=0x7fffffffbe68) at /home/cdodd/repos/p4org-p4c-alt/build/_deps/abseil-src/absl/container/internal/raw_hash_set.h:2423
#11 0x0000555558830503 in P4::Explain::postorder (this=0x7fffffffbe50, tv=0x7ffff77bd300)
    at /home/cdodd/repos/p4org-p4c-alt/frontends/p4/typeChecking/typeConstraints.h:42
#12 0x000055555898a1ae in IR::Type_Var::apply_visitor_postorder (this=0x7ffff77bd300, v=...) at /home/cdodd/repos/p4org-p4c-alt/ir/visitor.cpp:742
#13 0x000055555894a28c in Inspector::apply_visitor (this=0x7fffffffbe50, n=0x7ffff77bd300, name=0x55555720f5f9 "returnType")
    at /home/cdodd/repos/p4org-p4c-alt/ir/visitor.cpp:515
#14 0x0000555557eb3f3a in Visitor::visit (this=0x7fffffffbeb0, n=@0x7fffffffb880: 0x7ffff77bd300, name=0x55555720f5f9 "returnType")
    at /home/cdodd/repos/p4org-p4c-alt/ir/visitor.h:124
#15 0x00005555589cd88d in Visitor::visit (this=0x7fffffffbeb0, n=@0x7ffff77d8fc8: 0x7ffff77bd300, name=0x55555720f5f9 "returnType")
    at /home/cdodd/repos/p4org-p4c-alt/ir/visitor.cpp:764
#16 0x0000555557ce12db in IR::Type_MethodCall::visit_children (this=0x7ffff77d8f80, v=...)
    at /home/cdodd/repos/p4org-p4c-alt/build/ir/ir-generated.cpp:3681
#17 0x000055555894a26e in Inspector::apply_visitor (this=0x7fffffffbe50, n=0x7ffff77d8f80, name=0x0)
    at /home/cdodd/repos/p4org-p4c-alt/ir/visitor.cpp:514
#18 0x0000555557eb3ef2 in Visitor::visit (this=0x7fffffffbeb0, n=@0x7fffffffb998: 0x7ffff77d8f80, name=0x0)
    at /home/cdodd/repos/p4org-p4c-alt/ir/visitor.h:122
#19 0x0000555558930b4c in IR::Node::apply (this=0x7ffff77d8f80, v=..., ctxt=0x0) at /home/cdodd/repos/p4org-p4c-alt/ir/node.cpp:159
#20 0x0000555558830869 in P4::TypeConstraint::explain[abi:cxx11](unsigned long, P4::Explain*) const (this=0x7ffff77cb960, index=1, 
    explainer=0x7fffffffbe50) at /home/cdodd/repos/p4org-p4c-alt/frontends/p4/typeChecking/typeConstraints.h:77
#21 0x000055555882f5c0 in P4::TypeConstraint::localError[abi:cxx11](P4::Explain*) const (this=0x7ffff77cb960, explainer=0x7fffffffbe50)
    at /home/cdodd/repos/p4org-p4c-alt/frontends/p4/typeChecking/typeConstraints.cpp:137
#22 0x000055555882fe70 in P4::TypeConstraint::reportErrorImpl (this=0x7ffff77cb960, subst=0x7ffff77cb9c0, 
    message="  ---- Actual error:\n/home/cdodd/repos/p4org-p4c-alt/testdata/p4_16_errors/named-fail1.p4(8): No parameter named 'z'\n        f(z = b, y = b); // No such parameter\n          ^\n") at /home/cdodd/repos/p4org-p4c-alt/frontends/p4/typeChecking/typeConstraints.cpp:167
#23 0x00005555588403b2 in P4::TypeConstraint::reportError<IR::ID const&> (this=0x7ffff77cb960, subst=0x7ffff77cb9c0, 
    format=0x555557383cf3 "No parameter named '%1%'") at /home/cdodd/repos/p4org-p4c-alt/frontends/p4/typeChecking/typeConstraints.h:96
#24 0x000055555883ba55 in P4::TypeUnification::unifyCall (this=0x7ffff77c2a20, constraint=0x7ffff77cb960)
    at /home/cdodd/repos/p4org-p4c-alt/frontends/p4/typeChecking/typeUnification.cpp:71
#25 0x000055555883dead in P4::TypeUnification::unify (this=0x7ffff77c2a20, constraint=0x7ffff77cb960)
    at /home/cdodd/repos/p4org-p4c-alt/frontends/p4/typeChecking/typeUnification.cpp:308
#26 0x000055555882f089 in P4::TypeConstraints::solve (this=0x7fffffffc6e0, constraint=0x7ffff77cb960)
    at /home/cdodd/repos/p4org-p4c-alt/frontends/p4/typeChecking/typeConstraints.cpp:102
#27 0x000055555882e859 in P4::TypeConstraints::solve (this=0x7fffffffc6e0)
    at /home/cdodd/repos/p4org-p4c-alt/frontends/p4/typeChecking/typeConstraints.cpp:46
#28 0x00005555587b7add in P4::TypeInference::unifyBase (this=0x7ffff77a7620, allowCasts=false, errorPosition=0x7ffff77cf400, destType=0x7ffff77d2f30, 
    srcType=0x7ffff77d8f80, errorFormat="Function type '%1%' does not match invocation type '%2%'", errorArgs=std::initializer_list of length 2 = {...})
    at /home/cdodd/repos/p4org-p4c-alt/frontends/p4/typeChecking/typeChecker.cpp:186
#29 0x00005555587c14e2 in P4::TypeInference::unify (this=0x7ffff77a7620, errorPosition=0x7ffff77cf400, destType=0x7ffff77d2f30, srcType=0x7ffff77d8f80, 
    errorFormat="Function type '%1%' does not match invocation type '%2%'", errorArgs=std::initializer_list of length 2 = {...})
    at /home/cdodd/repos/p4org-p4c-alt/frontends/p4/typeChecking/typeChecker.h:132
#30 0x00005555587f04b9 in P4::TypeInference::postorder (this=0x7ffff77a7620, expression=0x7ffff77cf400)
    at /home/cdodd/repos/p4org-p4c-alt/frontends/p4/typeChecking/typeCheckExpr.cpp:1932
#31 0x000055555899e630 in IR::MethodCallExpression::apply_visitor_postorder (this=0x7ffff77cf400, v=...)
    at /home/cdodd/repos/p4org-p4c-alt/ir/visitor.cpp:742
#32 0x000055555894a8da in Transform::apply_visitor (this=0x7ffff77a7620, n=0x7ffff7828c00, name=0x5555572116b1 "methodCall")
    at /home/cdodd/repos/p4org-p4c-alt/ir/visitor.cpp:576

So the failing emplace is at typeConstraints.h line 42 in an Explain object being built for a type error in unification

[ChrisDodd]

If I back out just that change to typeConstraints.h (using absl::flat_hash_set instead of std::set), then the problem goes away.

[asl]

If I back out just that change to typeConstraints.h (using absl::flat_hash_set instead of std::set), then the problem goes away.

I think I know what happens and this is due to heterogenous lookup and overall IR structure. We are having:

class Type_Var : public Type_Declaration, public virtual ITypeVar

So, conversion of Type_Var to ITypeVar involves this-adjustment. There is an assert in emplace in abseil map that checks if stored value is equal to inserted one. And here it is not due to this cast to a second base class.

Given that we always store Type_Var to explained, maybe it would make sense to change explained to store const Type_Var* instead of const IR::ITypeVar *?

Another question is how you had asserts enabled for abseil build. We might want to have these in CI as well.

[ChrisDodd]

So, conversion of Type_Var to ITypeVar involves this-adjustment. There is an assert in emplace in abseil map that checks if stored value is equal to inserted one. And here it is not due to this cast to a second base class.

Given that we always store Type_Var to explained, maybe it would make sense to change explained to store const Type_Var* instead of const IR::ITypeVar *?

Makes sense -- or explicit (static_)cast to ITypeVar * in the emplace call? Either should work, though this is something to watch for flat_map use in the future.

Another question is how you had asserts enabled for abseil build. We might want to have these in CI as well.

I'm just using a normal -DCMAKE_BUILD_TYPE=DEBUG build, which is what I normally use for development -- the tests aren't too much slower in debug mode. I think CI uses RELEASE by default, but perhaps we should have both. It might also be good to have both with and without -DNDEBUG for other asserts (not sure what release builds do by default)

[asl]

Makes sense -- or explicit (static_)cast to ITypeVar * in the emplace call? Either should work, though this is something to watch for flat_map use in the future.

Yes, though it will be case for standard maps as well, as containers are starting to have heterogenous lookup from C++17 / C++20. In general the map itself is fine, it is just assertion being over-conservative.

Let me submit a PR then.

It might also be good to have both with and without -DNDEBUG for other asserts (not sure what release builds do by default)

I think tests always should run with assertions enabled, yes. @fruffy Can we enable this?