Skip to content

Fix Trac 6028: normalize SDCC assembly before preprocessing - #8892

Open
KiritoYG wants to merge 1 commit into
cppcheck-opensource:mainfrom
KiritoYG:codex/preprocess-sdcc-asm
Open

KiritoYG wants to merge 1 commit into
cppcheck-opensource:mainfrom
KiritoYG:codex/preprocess-sdcc-asm

Conversation

@KiritoYG

Copy link
Copy Markdown

Addresses the remaining SDCC examples in Trac 6028. @dptr operands currently cause a syntax error, and #(s_XINIT >> 8) is altered during preprocessing before the late assembler simplifier runs, leaving an unmatched ).

Normalize complete literal __asm / __endasm regions in the raw preprocessor tokens, including newly loaded headers. Preserve an opaque asm() analysis barrier, exactly one terminator and the locations of surrounding C code. Leave ambiguous/missing/nested regions, real preprocessing directives, macro definitions and other assembler dialects on the existing path. The existing pragma-asm handling is unchanged.

Add seven preprocessor test methods covering operands, empty/adjacent blocks, comments, missing/nested markers, other dialects, configurations and unbraced control flow. Two CLI regressions cover the main-file and included-header paths and verify that a real division-by-zero diagnostic after the block remains at its original line.

Validation:

  • The untouched base reproduces the remaining @ and #( failures; 18 original/patched CLI comparisons pass, including outside-C diagnostic and GNU/MS/macro controls.
  • Windows Clang/MinGW: 5,301 native tests, 351 existing TODOs, zero test failures; both new CLI tests pass.
  • Independent Linux GCC 11.4 verification of the exact source: warnings-as-errors build; 5,363 native tests, 360 existing TODOs, zero failures; both CLI tests pass. The helper branch adds only its verification workflow.
  • Uncrustify 0.80.1, Python lint and git diff --check pass.

One earlier Windows full-suite attempt exited with 0xc0000005 while displaying TestType::checkTooBigShift_Unix32; the full rerun above passed. The recorded fault offset resolves to the unchanged Token::Match. A bounded debugger run did not reproduce that failure but was stopped after becoming slow in TestValueFlow::valueFlowHang; its outcome is inconclusive. No root cause or baseline attribution is claimed.

This is a bounded fix for the demonstrated literal marker-delimited blocks. It does not repair initial lexer errors, macro-generated markers, or ARM assembler-function syntax.

Please assign Trac 6028 to KiritoYG for this patch and confirm whether this remaining-defect fix qualifies under the published USD 10 bounty bracket. After qualifying closure, I can use the documented bounty-request process; please also confirm the available settlement channel.

Comment thread lib/preprocessor.cpp
Comment on lines +1182 to +1185
// Preserve an asm statement as an analysis barrier, including for empty
// blocks. Hide assembler # operands before simplecpp stringifies them.
std::unique_ptr<simplecpp::Token> open(new simplecpp::Token("(", start->location));
std::unique_ptr<simplecpp::Token> close(new simplecpp::Token(")", start->location));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is an AI review. Take it with a grain of salt and feel free to reject it by resolving the comment.

I built the PR and compared it with main. Other asm forms are unchanged: __asm("..."), __asm volatile(...), MS __asm { ... }, one-line __asm nop __endasm;, and int x __asm("sym");. Unbraced if/do bodies with a block still parse. As a bonus, a { inside an asm ; comment no longer gives a syntax error.

One behavior change I noticed: the assembler text is now dropped. For

void f(void) {
    __asm
        mov ax,bx
    __endasm;
}

main produces asm ( "mov ax , bx" ) ; (from Tokenizer::simplifyAsm(), see testtokenize.cpp around line 1191). With this PR, the --debug output and the dump file show asm ( "" ) ;. I could not find any checker or addon that reads the asm string, and #pragma asm already produces an empty asm ( ), so this may be fine. But it means the __endasm branch in Tokenizer::simplifyAsm() is now effectively dead for preprocessed input. Is that intended? If so, maybe remove or document that branch. Otherwise, maybe keep the text here by turning the removed tokens into a string literal between ( and )?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think the assembler text is used somehow and shouldn't be dropped. However I am not sure what we use it for. One possible idea is when checking if the code in two scopes are the same to find copy paste mistakes, then we want to know if the same assembler instructions are provided.

for this purpose it's enough with a hash or an id string we don't need to see the exact code..

@dmcppcheck

Copy link
Copy Markdown
Contributor

Test results for commit b66fae3 (tools/test-my-pr.py, main compared to this PR):

Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8892/
packages: 200
Differing warnings: 0 (+0 -0)
Timing: +0.1% (http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8892/2026-09-24_15-59-20-b66fae3a3481_timing.html)

Posted automatically by the cppcheck PR test runner. +N: warnings only with this PR, -N: warnings only with main. The AI review is written by Claude and can be wrong.

@dmcppcheck dmcppcheck 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.

AI review

The fix works for the cases it covers. A block with @/#( operands that starts on its own line (or right after {, } or ;) no longer gives a syntax error, and diagnostics after it keep their original line. I found one new gap. If __asm comes on the same line as a statement prefix such as if (a), else or case 1:, it is not normalized, so the same syntax errors as on main remain. This includes the unbraced if/else case that the PR says it covers, as soon as __asm is not on a line of its own. The dropped assembler text was already raised in comments.md, so I don't repeat it. I found no false positives, crashes or line-number shifts.

Testing

Built cppcheck and testrunner in pr/ and cppcheck in base/ without errors.

  • ./testrunner TestPreprocessor TestTokenizer: 0 failures.
  • pytest test/cli/other_test.py -k sdcc: 2 passed.

Scratch comparisons, base vs PR:

  • Fixed by the PR: two blocks with #(s_XINIT>>8) in one function, __endasm with a trailing // comment and ; on the next line, a __naked function, and __endasm ; int y = 1/0; on one line. Base gives Unmatched ')'. The PR reports the zerodiv diagnostics at the correct lines.
  • Unchanged from main, as intended: a #define with __asm/__endasm over continuation lines, and a struct member named __asm.

Not fixed: __asm after a statement prefix on the same line

// c4.c
void f(int a) {
    switch (a) {
    case 1: __asm
        movx @dptr,a
    __endasm;
    break;
    }
}
// c5.c
void f(int a) {
    if (a) __asm
        mov a,#(s_XINIT>>8)
    __endasm;
    else a = 1/0;
}

Command: cppcheck -q --template='{line}:{id}:{message}' c4.c c5.c. Base and PR give the same output:

4:syntaxError:syntax error
3:syntaxError:Unmatched ')'. Configuration: ''.

With __asm on its own line, both files are handled correctly.


Automated review of b66fae3 by Claude. The pull request was built and tested locally.

Comment thread lib/preprocessor.cpp
{
simplifyPragmaAsm(mTokens);
const simplecpp::Token* previous = start->previousSkipComments();
if (sameline(start, previous) && previous->op != '{' && previous->op != '}' && previous->op != ';')

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.

AI review

This check only accepts __asm at the start of a line or right after {, } or ;. Code such as if (a) __asm, else __asm, case 1: __asm or label: __asm, followed by movx @dptr,a / mov a,#(s_XINIT>>8) on the next lines, still gives syntax error / Unmatched ')', the same as on main. I reproduced this with case 1: __asm and if (a) __asm.

The GNU/ARM forms that this check guards against (int x __asm("sym"), __asm void g(), __asm volatile) are already rejected by the first checks below. Could this also accept a previous ), : or else, or be dropped in favour of the first checks?

@danmar

danmar commented Nov 7, 2026

Copy link
Copy Markdown
Collaborator

hi sorry for late reply.. could you indicate if you feel that the AI comments should be fixed? it looks reasonable to me but if you feel it's out-of-scope for this PR then we can add tickets and merge this PR..

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.

3 participants