Standardize logging utility #552

ChinYikMing · 2025-02-03T15:29:17Z

Previously, messages were printed using both printf and fprintf,
causing the information to be mixed between stdout and stderr. To
address this, the log.[ch] was integrated to standardize the logging.
The log.[ch]'s API are encapsulated one more layer with prefix 'rv',
and included in src/common.h.

The logging API uses color to differentiate messages at different
levels, ensuring that logging all information to the same stdout stream
does not cause confusion. The color feature is controlled by
ENABLE_LOG_COLOR and is enabled by default.

Note that the logging stdout stream is registered during
rv_remap_stdstream() as the new stdout stream could be remapped at
there.

Since the newly introduced logging APIs generate logs during runtime,
the check recipe must filter out this log information before validation.

Additionally, refine the check recipe into a new target, check-test,
which serves as a template to enhance readability.

Summary by Bito

This PR implements a unified logging system that replaces scattered printf/fprintf calls with a standardized API supporting multiple severity levels and color-coded output. The system is integrated across the RISC-V emulator core, device emulation, and system calls, with the build system updated to handle filtered logging output and improved test recipes.

Unit tests added: False

Estimated effort to review (1-5, lower is better): 5

bito-code-review · 2025-02-03T15:29:39Z

Code Review Agent Run #571ed1

Actionable Suggestions - 9

src/riscv.c - 1
- Consider proper cleanup before exit · Line 427-428
src/syscall.c - 1
- Consider using fatal error for syscalls · Line 495-495
src/syscall_sdl.c - 2
- Consider adding request value to error · Line 1010-1010
- Consider adding request value to error · Line 1034-1034
src/log.c - 1
- Consider larger buffer for time format · Line 55-55
src/log.h - 1
- Incorrect function reference in macro definition · Line 61-61
Makefile - 3
- Consider validating mktemp command output · Line 354-356
- Consider adding detailed test failure output · Line 367-370
- Consider maintaining original output scope · Line 392-392

Review Details

Files reviewed - 15 · Commit Range: a841b40..d8912c4
- Makefile
- src/common.h
- src/devices/uart.c
- src/devices/virtio-blk.c
- src/emulate.c
- src/feature.h
- src/log.c
- src/log.h
- src/main.c
- src/riscv.c
- src/riscv.h
- src/syscall.c
- src/syscall_sdl.c
- src/system.c
- src/t2c.c
Files skipped - 0
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Fb Infer (Static Code Analysis) - ✖︎ Failed

AI Code Review powered by

jserv

Benchmarks

Benchmark suite	Current: `85701c4`	Previous: `9e07773`	Ratio
`Dhrystone`	`1307` Average DMIPS over 10 runs	`1330` Average DMIPS over 10 runs	`1.02`
`Coremark`	`969.939` Average iterations/sec over 10 runs	`962.043` Average iterations/sec over 10 runs	`0.99`

This comment was automatically generated by workflow using github-action-benchmark.

bito-code-review · 2025-02-03T15:44:42Z

Changelist by Bito

This pull request implements the following key changes.

Key Change	Files Impacted
Feature Improvement - Standardized Logging System Implementation	- `log.h` - Added new logging header with severity levels and color support - `log.c` - Implemented core logging functionality with color-coded output - `common.h` - Integrated logging utility headers - `riscv.h` - Added logging support and configuration - `riscv.c` - Replaced printf/fprintf with standardized logging calls - `uart.c` - Updated error messages to use new logging system - `virtio-blk.c` - Converted error reporting to use logging API - `syscall.c` - Migrated system messages to logging framework - `syscall_sdl.c` - Updated SDL-related messages to use logging system - `system.c` - Converted MMIO error messages to logging API - `t2c.c` - Updated error handling to use logging framework - `emulate.c` - Integrated logging system for emulator messages - `main.c` - Added logging initialization and usage
Testing - Test Infrastructure Enhancement	- `Makefile` - Added log filtering and improved test recipe readability with check-test template

src/riscv.c

src/syscall.c

src/syscall_sdl.c

src/log.c

src/log.h

Makefile

bito-code-review · 2025-02-03T16:17:53Z

Code Review Agent Run #608943

Actionable Suggestions - 7

src/syscall_sdl.c - 2
- Consider adding error handling for unknown request · Line 1034-1034
- Consider consistent error handling pattern · Line 923-941
src/main.c - 1
- Consider adding error return value · Line 191-191
src/riscv.c - 2
- Consider relocating stdout stream setup call · Line 169-169
- Consider more descriptive error message · Line 260-260
src/log.c - 2
- Consider standard ternary operator syntax · Line 160-160
- Consider optimizing va_list handling in loop · Line 171-173

Additional Suggestions - 1

Makefile - 1
- Consider adding detailed test failure output · Line 388-388

Review Details

Files reviewed - 15 · Commit Range: 7c9d75e..517c23d
- Makefile
- src/common.h
- src/devices/uart.c
- src/devices/virtio-blk.c
- src/emulate.c
- src/feature.h
- src/log.c
- src/log.h
- src/main.c
- src/riscv.c
- src/riscv.h
- src/syscall.c
- src/syscall_sdl.c
- src/system.c
- src/t2c.c
Files skipped - 0
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Fb Infer (Static Code Analysis) - ✖︎ Failed

AI Code Review powered by

src/main.c

src/riscv.c

src/syscall_sdl.c

src/log.c

bito-code-review · 2025-02-03T16:40:54Z

Code Review Agent Run #d67d08

Actionable Suggestions - 6

src/syscall_sdl.c - 2
- Consider error handling for unknown request · Line 1010-1010
- Consider adding error handling for invalid request · Line 1034-1034
Makefile - 1
- Consider adding SHA1SUM command error handling · Line 367-370
src/main.c - 2
- Consider enhancing error handling details · Line 191-191
- Consider enhancing error handling details · Line 191-191
src/t2c.c - 1
- Consider consistent error handling pattern · Line 309-309

Additional Suggestions - 4

src/log.h - 2
- Consider adding LOG_NONE level for disabling · Line 43-50
- Consider const qualifier for time field · Line 34-34
src/syscall_sdl.c - 1
- Consider consistent error handling approach · Line 257-265
src/devices/uart.c - 1
- Consider null check for strerror result · Line 50-50

Review Details

Files reviewed - 15 · Commit Range: 517e17b..da1fa29
- Makefile
- src/common.h
- src/devices/uart.c
- src/devices/virtio-blk.c
- src/emulate.c
- src/feature.h
- src/log.c
- src/log.h
- src/main.c
- src/riscv.c
- src/riscv.h
- src/syscall.c
- src/syscall_sdl.c
- src/system.c
- src/t2c.c
Files skipped - 0
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Fb Infer (Static Code Analysis) - ✖︎ Failed

AI Code Review powered by

src/main.c

src/t2c.c

bito-code-review · 2025-02-03T16:58:41Z

Code Review Agent Run #343259

Actionable Suggestions - 4

src/syscall_sdl.c - 1
- Consider adding error handling for unknown request · Line 1034-1034
Makefile - 2
- Consider adding early exit on test failure · Line 380-380
- Consider checking both output lines · Line 392-392
src/log.c - 1
- Consider explicit initialization of time field · Line 150-155

Additional Suggestions - 5

Makefile - 3
- Consider adding detailed error output · Line 370-370
- Consider more descriptive macro parameters · Line 388-388
- Consider adding file existence check · Line 384-384
src/system.c - 1
- Consider adding more context to error · Line 98-98
src/log.h - 1
- Consider adding semicolon after enum definition · Line 43-50

Review Details

Files reviewed - 15 · Commit Range: c2a3185..10ca95d
- Makefile
- src/common.h
- src/devices/uart.c
- src/devices/virtio-blk.c
- src/emulate.c
- src/feature.h
- src/log.c
- src/log.h
- src/main.c
- src/riscv.c
- src/riscv.h
- src/syscall.c
- src/syscall_sdl.c
- src/system.c
- src/t2c.c
Files skipped - 0
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Fb Infer (Static Code Analysis) - ✖︎ Failed

AI Code Review powered by

Makefile

bito-code-review · 2025-02-03T17:36:50Z

Code Review Agent Run #e5b0db

Actionable Suggestions - 2

src/syscall_sdl.c - 1
- Consider error handling for unknown request · Line 1034-1034
Makefile - 1
- Consider adding detailed test failure output · Line 375-378

Additional Suggestions - 4

src/system.c - 1
- Consider adding address to MMIO error · Line 98-98
src/log.h - 1
- Consider consolidating duplicate logging macros · Line 53-58
src/main.c - 1
- Consider consistent log levels for lifecycle events · Line 278-278
src/devices/virtio-blk.c - 1
- Consider consolidating duplicate error messages · Line 411-417

Review Details

Files reviewed - 15 · Commit Range: 934c21d..e759f8e
- Makefile
- src/common.h
- src/devices/uart.c
- src/devices/virtio-blk.c
- src/emulate.c
- src/feature.h
- src/log.c
- src/log.h
- src/main.c
- src/riscv.c
- src/riscv.h
- src/syscall.c
- src/syscall_sdl.c
- src/system.c
- src/t2c.c
Files skipped - 1
- README.md - Reason: Filter setting
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Fb Infer (Static Code Analysis) - ✖︎ Failed

AI Code Review powered by

Makefile

src/log.c

jserv · 2025-02-03T21:46:44Z

src/main.c

-            "RV32I[MAFC] Emulator which loads an ELF file to execute.\n"
-            "Usage: %s [options] [filename] [arguments]\n"
-            "Options:\n"
+    rv_log_info(


This message should always appear, so that INFO is not sufficient.

This message should always appear, so that INFO is not sufficient.

Use TRACE instead.

Use TRACE instead.

How about ERROR instead? While dumping the usage, the program exits with non-zero code.

Use TRACE instead.

How about ERROR instead? While dumping the usage, the program exits with non-zero code.

I chose wrong level (TRACE) which is the lowest log level. I originally planned to choose highest log level (FATAL), but ERROR works as well, so I'll go with that.

Makefile

jserv

Since the logging feature is quite useful, we can promote the functions as public APIs. That is, include log.h in the header riscv.h.

bito-code-review · 2025-02-04T05:53:05Z

Code Review Agent Run #648fc0

Actionable Suggestions - 7

src/syscall_sdl.c - 1
- Consider adding error handling for invalid request · Line 1034-1034
src/log.c - 2
- Consider adding localtime error handling · Line 96-97
- Consider adding FILE* type validation · Line 118-119
Makefile - 1
- Consider impact of tail line count change · Line 392-392
src/main.c - 2
- Consider NULL check for signature_out_file · Line 191-192
- Consider consistent log severity levels · Line 278-278
src/devices/virtio-blk.c - 1
- Consider consolidating duplicate error messages · Line 411-417

Review Details

Files reviewed - 15 · Commit Range: 57c74da..fee21ca
- Makefile
- src/common.h
- src/devices/uart.c
- src/devices/virtio-blk.c
- src/emulate.c
- src/feature.h
- src/log.c
- src/log.h
- src/main.c
- src/riscv.c
- src/riscv.h
- src/syscall.c
- src/syscall_sdl.c
- src/system.c
- src/t2c.c
Files skipped - 1
- README.md - Reason: Filter setting
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Fb Infer (Static Code Analysis) - ✖︎ Failed

AI Code Review powered by

src/log.c

Makefile

src/main.c

src/devices/virtio-blk.c

bito-code-review · 2025-02-04T06:10:12Z

Code Review Agent Run #d970f8

Actionable Suggestions - 4

src/syscall_sdl.c - 1
- Consider adding error handling for invalid request · Line 1034-1034
src/main.c - 2
- Consider using fatal error logging level · Line 191-192
- Consider using fatal error logging level · Line 191-192
src/riscv.c - 1
- Consider configurable log level setting · Line 416-419

Additional Suggestions - 1

src/log.h - 1
- Consider reducing macro code duplication · Line 53-58

Review Details

Files reviewed - 15 · Commit Range: 4667fc3..9e07773
- Makefile
- src/common.h
- src/devices/uart.c
- src/devices/virtio-blk.c
- src/emulate.c
- src/feature.h
- src/log.c
- src/log.h
- src/main.c
- src/riscv.c
- src/riscv.h
- src/syscall.c
- src/syscall_sdl.c
- src/system.c
- src/t2c.c
Files skipped - 1
- README.md - Reason: Filter setting
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Fb Infer (Static Code Analysis) - ✖︎ Failed

AI Code Review powered by

src/main.c

src/riscv.c

Previously, messages were printed using both printf and fprintf, causing the information to be mixed between stdout and stderr. To address this, the log.[ch] was integrated to standardize the logging. The log.[ch]'s API are encapsulated one more layer with prefix 'rv', and included in src/common.h. The logging API uses color to differentiate messages at different levels, ensuring that logging all information to the same stdout stream does not cause confusion. The color feature is controlled by ENABLE_LOG_COLOR and is enabled by default. Note that the logging stdout stream is registered during rv_remap_stdstream() as the new stdout stream could be remapped at there. Related: sysprog21#310

Since the newly introduced logging APIs generate logs during runtime, the check recipe must filter out this log information before validation. Additionally, refine the check recipe into a new target, check-test, which serves as a template to enhance readability.

bito-code-review · 2025-02-04T06:47:23Z

Code Review Agent Run #d90a76

Actionable Suggestions - 1

src/syscall_sdl.c - 1
- Consider error handling for unknown request · Line 1034-1034

Additional Suggestions - 4

src/riscv.c - 1
- Consider consistent log levels for errors · Line 873-873
src/system.c - 1
- Consider enhancing MMIO error message details · Line 98-98
src/log.h - 1
- Consider wrapping macros in do-while · Line 53-58
src/feature.h - 1
- Consider more descriptive feature flag name · Line 111-114

Review Details

Files reviewed - 15 · Commit Range: 2e87a75..85701c4
- Makefile
- src/common.h
- src/devices/uart.c
- src/devices/virtio-blk.c
- src/emulate.c
- src/feature.h
- src/log.c
- src/log.h
- src/main.c
- src/riscv.c
- src/riscv.h
- src/syscall.c
- src/syscall_sdl.c
- src/system.c
- src/t2c.c
Files skipped - 1
- README.md - Reason: Filter setting
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Fb Infer (Static Code Analysis) - ✖︎ Failed

AI Code Review powered by

jserv · 2025-02-04T08:56:42Z

Thank @ChinYikMing for contributing!

ChinYikMing changed the title ~~Feat/logging api~~ Standardize logging utility Feb 3, 2025

jserv reviewed Feb 3, 2025

View reviewed changes