Unify debug / user-side node stringification

[asl]

we probably want another DBPrint flag here to disable the printing of mangled names/only print origName as we're using this for toString which is user facing. Improvement unrelated to this change really.

Originally posted by @ChrisDodd in #4686 (comment)

[asl]

testdata/p4_16_samples_outputs/array_field.p4-stderr is a great example where we produce something internal in the warnings which might be confusing to users

[asl]

So, things are a bit messy and it manifests in different weird ways (see e.g. #4825 (comment))

In fact, we are having multiple ways to "print something" inside p4c:

• There is toString method
• There is dbprint method (and some convenient things like dbp, etc.)
• There is operator<<

User-side error reporting relies on toString almost entirely. Both dbprint and operator<< are expected to be used for debug output only (e.g. in BUG_CHECK). However, this usage is quite error prone:

• Boost::Format relies on operator<< for user-defined types. So, this means that some accidental use of boost::fmt might expose debugging output
• We do define operator<< overload in stringify.h with the following convention: if there is dbprint method, then call it, otherwise try toString. This is quite error prone as this operator is hidden from ADL.
• operator<< is also used in logger and some existing IR node toString / dbprint code rely on operator<< of members.

I am proposing the following refactoring:

• Remove "generic" operator<< overloads
• Teach bug helper to rely on dbprint or operator<< for "ordinary (e.g. integers, pointers, etc.) types

This will break logger, but it seems it could be solved via creating our own logger stream and providing proper operator<<(P4C::log_stream&, T) overload. The IR node usage of operator<< could also be fixed as required.

[fruffy]

I am proposing the following refactoring:

• Remove "generic" operator<< overloads

• Teach bug helper to rely on dbprint or operator<< for "ordinary (e.g. integers, pointers, etc.) types

Seems good to me. Iirc, dbprint prints the full content of the "node" whereas toString is often a truncated representation. But the boundaries between those two are quite blurry and often toString has the same behavior as dbprint.

[asl]

Seems good to me. Iirc, dbprint prints the full content of the "node" whereas toString is often a truncated representation. But the boundaries between those two are quite blurry and often toString has the same behavior as dbprint.

Right. In same cases (e.g. for IR::Expression) toString is implemented in terms of dbprint showing unprepared users unintended output (something like arr[a/a_0+1]).

[vlstill]

btw. there is more related mess with operator<< e.g. there is an overload for std::vector<T> and std::set<T> in lib/log.h.

Another problem is in use of these operators in constructs like ASSUME_EQ(a, b) << expr which makes using P4::operator<< in the function as a workaround ineffective because the operator<< called in this expression is actually the one from GTest, which calls global/ADL operator<<.

[asl]

btw. there is more related mess with operator<< e.g. there is an overload for std::vector<T> and std::set<T> in lib/log.h.

Yes, exactly. .. and few other smallish cases here and there.

[vlstill]

This will break logger, but it seems it could be solved via creating our own logger stream and providing proper operator<<(P4C::log_stream&, T) overload. The IR node usage of operator<< could also be fixed as required.

It would be useful to have something like

log_stream<Str> operator<<(Str &, EnterLogStreamType);

so that one can enable logging into GTest asserts like this:

ASSERT_EQ(...) << P4::enterLogStream() << node;

This should all be well defined and findable by ADL.

[asl]

This should all be well defined and findable by ADL.

Oh, right, I completely forgot about that gtest feature with custom fail messages!