Skip to content

fix: Improve logging controls and robust integer literal generation#280

Merged
kumarak merged 6 commits into
mainfrom
kumarak/fix_logging
Jun 7, 2026
Merged

fix: Improve logging controls and robust integer literal generation#280
kumarak merged 6 commits into
mainfrom
kumarak/fix_logging

Conversation

@kumarak

@kumarak kumarak commented Jun 6, 2026

Copy link
Copy Markdown
Collaborator

This pull request improves logging configuration and fixes several robustness issues related to integer literal generation in AST operations.

Key changes include:

  • Adding a glog-style minimum log level to suppress low-priority logs unless verbose mode is enabled.
  • Updating the LOG macro and main.cpp to respect the configured verbosity level.
  • Introducing make_int_literal_printable, a helper that safely creates printable integer literals for problematic types such as _Bool, char8_t, and enums.
  • Updating integer literal creation sites in OperationStmt.cpp to use the new helper, preventing AST printer crashes and improving type handling consistency.

kumarak added 5 commits June 6, 2026 11:57
Zero/sentinel IntegerLiterals typed from an operand (_Bool, char8/16/32, or a
non-builtin) tripped StmtPrinter's llvm_unreachable("Unexpected type for integer
literal") during -print-tu. make_int_literal_printable remaps such literals to a
same-width standard integer; comparisons/assignments convert as usual.
…g-style)

LOG() now checks a global MinLogLevel (default WARNING); --verbose lowers it to
DEBUG. WARNING/ERROR/FATAL always emit. Removes the [INFO] spam from normal runs.
createDefaultArgument synthesized a zero typed param_type for any isIntegerType()
param -- including _Bool/enum/char8_t, which crash StmtPrinter. Route through
make_int_literal_printable. Completes the integer-literal unreachable fix.
Addresses PR #280 review:
- #1: promote the printable-literal helper to Utils (CamelCase MakeIntLiteralPrintable)
  and route the switch case-value literals (integer/address switch in create_branchind
  and BuildSNodeFromRegion) through it, so a _Bool/char32_t discriminant can't trip
  StmtPrinter. Drops now-unused disc_width/case_width.
- #2: guard the helper with isIntegralOrEnumerationType before getIntWidth (falls back
  to int for non-integer operand types).
- #3: rename to CamelCase per clang-tidy.
@kumarak

kumarak commented Jun 6, 2026

Copy link
Copy Markdown
Collaborator Author

@claude

@claude

claude Bot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kumarak's task in 8m 3s —— View job


PR Review — #280

  • Check scope (6 C/C++ files in scope)
  • Fetch PR diff
  • Fetch PR head SHA (8f3a0ae)
  • Read changed files at PR head
  • Review each changed file
  • Post inline findings (none)
  • Deliver final verdict — approved

No blocking issues found. Reviewed Log.hpp, Utils.cpp/hpp, OperationStmt.cpp, BuildSNodeFromRegion.cpp, and main.cpp. Key points confirmed correct:

  • LOG macro suppression: MinLogLevel() defaults to WARNING (2); FATAL (4) and ERROR (3) are never suppressed under any setting reachable via the CLI, preserving crash diagnostics.
  • MakeIntLiteralPrintable: type-replacement path is entered only when printable() returns false; printable() is only called when operand_type is non-null (guarded by the integral check), so no null-dereference risk. Fallback to 32-bit int for 1-bit _Bool is correct — all call sites pass values 0 or 1 for bool discriminators. APInt isSigned has no effect on stored bits for widths ≤ 64, so sign semantics are unchanged.
  • Enum lowering before literal creation: both OperationStmt.cpp and BuildSNodeFromRegion.cpp lower enum discriminator types to their underlying integer type before calling MakeIntLiteralPrintable, avoiding the getIntWidth-on-enum assertion path.
  • disc_width removal: correctly eliminated because MakeIntLiteralPrintable internally calls ctx.getIntWidth(lit_type) after selecting the final type.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

The global default of WARNING suppressed INFO for every tool, but only
patchir-decomp wires --verbose to re-enable it -- breaking patchir-transform's
yaml-parser tests that grep for the INFO 'Successfully parsed' line (CI failure
in PR #280). Default DEBUG restores historical behavior for tools that don't opt
in; patchir-decomp still raises to WARNING (INFO off) unless --verbose.
@kumarak
kumarak merged commit 039e560 into main Jun 7, 2026
13 checks passed
@kumarak
kumarak deleted the kumarak/fix_logging branch June 7, 2026 00:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant