feat(avm)!: optionally use TS logger in C++ simulation#19305
Conversation
fcarreiro
changed the title
fcarreiro
force-pushed
the
fc/cpp-log-through-ts
branch
from
80956ce to
b2a20fc
Compare
fcarreiro
force-pushed
the
fc/cpp-log-through-ts
branch
from
b2a20fc to
cc54b5f
Compare
Flakey Tests🤖 says: This CI run detected 1 tests that failed, but were tolerated due to a .test_patterns.yml entry. |
| enum class LogLevel : int { | ||
| DEBUG = 0, | ||
| INFO = 1, | ||
| VERBOSE = 2, |
There was a problem hiding this comment.
note to self: move VERBOSE, reorder
There was a problem hiding this comment.
I know we simply don't have logging functions for all levels in C++, but the asymmetry in log-levels between TS and C++ is unfortunate.
dbanks12
left a comment
dbanks12
left a comment
There was a problem hiding this comment.
LGTM with a couple comments
| enum class LogLevel : int { | ||
| DEBUG = 0, | ||
| INFO = 1, | ||
| VERBOSE = 2, |
There was a problem hiding this comment.
I know we simply don't have logging functions for all levels in C++, but the asymmetry in log-levels between TS and C++ is unfortunate.
| debug_logging = (log_level >= LOG_LEVEL_TRACE); | ||
| } | ||
|
|
||
| // Map C++ LogLevel enum to TypeScript log level string | ||
| // C++ LogLevel: DEBUG=0, INFO=1, VERBOSE=2, IMPORTANT=3 | ||
| // TS LogLevels: ['silent', 'fatal', 'error', 'warn', 'info', 'verbose', 'debug', 'trace'] | ||
| inline const char* cpp_log_level_to_ts(LogLevel level) | ||
| { | ||
| switch (level) { | ||
| case LogLevel::DEBUG: | ||
| return "debug"; | ||
| case LogLevel::INFO: | ||
| return "info"; | ||
| case LogLevel::VERBOSE: | ||
| return "verbose"; | ||
| case LogLevel::IMPORTANT: | ||
| return "warn"; | ||
| default: | ||
| return "info"; | ||
| } | ||
| } |
There was a problem hiding this comment.
Will this work oddly because above we set debug_logging = (log_level >= TRACE) and then later we set treat debug() and then in this switch-case we map DEBUG->debug? I'd suggest we just have debug == DEBUG, but then I think we need "trace" for per-execution-iteration prints (like the debug that prints the pc & opcode).
There was a problem hiding this comment.
I see your point. I think I'll leave it as it is right now, and try to convince @ludamad to let me align BB and TS log levels.
Merge activity
|
BEGIN_COMMIT_OVERRIDE feat(avm)!: optionally use TS logger in C++ simulation (#19305) chore(avm): bytecode caching comments chore(avm): disable VK hash checking in tests fix(avm)!: instr_fetching soundness bug (#19381) fix(avm): dont catch wide exceptions (#19388) refactor(avm): Refactor get contract instance fuzzer backfill (#19387) feat(avm): mutate enqueued calls (#19315) chore(avm): migrate to BB asserts (#19395) fix!: more missing boolean constraints in calldata, calldata hashing, sha256 mem PILs (#19367) feat(avm): defensively assert cd hashes (#19346) chore: annotate booleans in PIL, and add some missing boolean constraints (#19371) fix!: missing boolean constraints on zero checks targets (#19401) fix!: context did not constrain returndata size to 0 at start, and had a misnamed relation (#19404) END_COMMIT_OVERRIDE
5 participants
This PR
info/vinfo/etc customizable by a logging functionThe TS-side of the CPP simulator now uses the pino logger.
I've also refactored the
avm_simulate_napi.cppfile to avoid referencing the parameter indices many times, and using them in a very "local" way. Otherwise it was getting a bit dangerous that we would use some magic index in places many lines away.This shouldn't cause any overhead if logging is not active (as expected in block building, because ANY logging might take a
ms- e.g., showing the transaction hash). If you are doing any logging, you will pay dearly.Closes AVM-182.