Skip to content

Fix Trac 6028: normalize SDCC assembly before preprocessing #8892

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
KiritoYG wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix Trac 6028: normalize SDCC assembly before preprocessing #8892

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/cppcheck.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1032,7 +1032,7 @@ unsigned int CppCheck::checkInternal(const FileWithDetails& file, const std::str
// Get directives
std::list<Directive> directives;
preprocessor.createDirectives(directives);
preprocessor.simplifyPragmaAsm();
preprocessor.simplifyAsm();

std::set<std::string> configurations;
std::set<std::string> configDefines = { "__cplusplus" };
Expand Down Expand Up @@ -1060,7 +1060,7 @@ unsigned int CppCheck::checkInternal(const FileWithDetails& file, const std::str
if (!mSettings.keepComments)
Preprocessor::removeComments(data.tokens);
Preprocessor::createDirectives(data.tokens, directives);
Preprocessor::simplifyPragmaAsm(data.tokens);
Preprocessor::simplifyAsm(data.tokens);
// Discover new configurations from included file
if (configurations.size() < maxConfigs)
preprocessor.getConfigs(data.filename, data.tokens, configDefines, configurations);
Expand Down
80 changes: 77 additions & 3 deletions lib/preprocessor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1133,15 +1133,89 @@ std::size_t Preprocessor::calculateHash(const std::string &toolinfo) const
return (std::hash<std::string>{})(hashData);
}

void Preprocessor::simplifyPragmaAsm()
static simplecpp::Token* simplifySdccAsm(simplecpp::TokenList& tokenList, simplecpp::Token* start)
{
simplifyPragmaAsm(mTokens);
const simplecpp::Token* previous = start->previousSkipComments();
if (sameline(start, previous) && previous->op != '{' && previous->op != '}' && previous->op != ';')
return nullptr;
// Do not rewrite a macro definition or a GNU/MS-style asm statement.
for (const simplecpp::Token* tok = previous; sameline(start, tok); tok = tok->previous) {
if (tok->op == '#')
return nullptr;
}
const simplecpp::Token* first = start->nextSkipComments();
if (!first || (sameline(start, first) && first->op != ';') ||
first->op == '(' || first->op == '{' || first->str() == "volatile" ||
first->str() == "__volatile" || first->str() == "__volatile__" ||
first->str() == "goto" || first->str() == "inline")
return nullptr;

simplecpp::Token* end = start->next;
const simplecpp::Token* comment = nullptr;
bool nestedAsm = false;
for (; end; end = end->next) {
if (end->comment || sameline(end, comment))
continue;
if (end->op == ';') {
comment = end;
continue;
}
if (end->str() == "__endasm" && !sameline(end, end->previousSkipComments())) {
const simplecpp::Token* next = end->nextSkipComments();
if (!sameline(end, next) || next->op == ';')
break;
}
// An incomplete block must not consume C code or a different conditional
// branch. Leave such input to the normal preprocessor/tokenizer.
if (end->str() == "__asm")
nestedAsm = true;
if (end->op == '{' || end->op == '}' ||
(end->op == '#' && !sameline(end, end->previousSkipComments())))
return end->previous;
}
if (!end)
return tokenList.back();
// Do not normalize an inner block after rejecting an ambiguous outer one.
if (nestedAsm)
return end;

// 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));
Comment on lines +1182 to +1185

@danmar danmar Oct 1, 2026

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 )?

// A second terminator would break an unbraced do/while or if/else body.
simplecpp::Token* terminator = end->next;
while (terminator && terminator->comment)
terminator = terminator->next;
if (terminator && terminator->op == ';')
tokenList.deleteToken(terminator);
start->setstr("asm");
end->setstr(";");
while (start->next != end)
tokenList.deleteToken(start->next);
open->previous = start;
open->next = close.get();
close->previous = open.get();
close->next = end;
start->next = open.release();
end->previous = close.release();
return end;
}

void Preprocessor::simplifyAsm()
{
simplifyAsm(mTokens);
}

void Preprocessor::simplifyPragmaAsm(simplecpp::TokenList &tokenList)
void Preprocessor::simplifyAsm(simplecpp::TokenList &tokenList)
{
// assembler code..
for (simplecpp::Token *tok = tokenList.front(); tok; tok = tok->next) {
if (tok->str() == "__asm") {
if (simplecpp::Token* end = simplifySdccAsm(tokenList, tok))
tok = end;
continue;
}
if (tok->op != '#')
continue;
if (sameline(tok, tok->previousSkipComments()))
Expand Down
4 changes: 2 additions & 2 deletions lib/preprocessor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,9 +144,9 @@ class CPPCHECKLIB WARN_UNUSED Preprocessor {
*/
std::size_t calculateHash(const std::string &toolinfo) const;

void simplifyPragmaAsm();
void simplifyAsm();

static void simplifyPragmaAsm(simplecpp::TokenList &tokenList);
static void simplifyAsm(simplecpp::TokenList &tokenList);

static void getErrorMessages(ErrorLogger &errorLogger, const Settings &settings);

Expand Down
19 changes: 19 additions & 0 deletions test/cli/other_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,25 @@ def test_preprocessor_error(tmpdir):
assert exitcode != 0


@pytest.mark.parametrize('in_header', [False, True])
def test_sdcc_asm_operands(tmp_path, in_header): # #6028
source = ('int f(void) {\n'
' __asm\n'
' movx @dptr,a\n'
' mov b,#(s_XINIT>>8)\n'
' __endasm;\n'
' return 1/0;\n'
'}\n')
main_file = tmp_path / 'main.c'
asm_file = tmp_path / 'asm.h' if in_header else main_file
asm_file.write_text(source)
if in_header:
main_file.write_text('#include "asm.h"\n')
exitcode, _, stderr = cppcheck(['--error-exitcode=1', '--template={file}:{line}:{id}', str(main_file)])
assert exitcode == 1
assert stderr == '{}:6:zerodiv\n'.format(asm_file)


__ANSI_BOLD = "\x1b[1m"
__ANSI_FG_RED = "\x1b[31m"
__ANSI_FG_DEFAULT = "\x1b[39m"
Expand Down
154 changes: 153 additions & 1 deletion test/testpreprocessor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ class TestPreprocessor : public TestFixture {
if (inlineSuppression)
preprocessor.inlineSuppressions(*inlineSuppression);
preprocessor.removeComments();
preprocessor.simplifyPragmaAsm();
preprocessor.simplifyAsm();

std::map<std::string, std::string> cfgcode;
if (cfgs.empty()) {
Expand Down Expand Up @@ -255,6 +255,13 @@ class TestPreprocessor : public TestFixture {
TEST_CASE(pragma);
TEST_CASE(pragma_asm_1);
TEST_CASE(pragma_asm_2);
TEST_CASE(sdccAsmOperands);
TEST_CASE(sdccAsmEmptyAndAdjacent);
TEST_CASE(sdccAsmComments);
TEST_CASE(sdccAsmIncomplete);
TEST_CASE(sdccAsmOtherSyntax);
TEST_CASE(sdccAsmConditional);
TEST_CASE(sdccAsmControlFlow);
TEST_CASE(endifsemicolon);
TEST_CASE(missing_doublequote);
TEST_CASE(handle_error);
Expand Down Expand Up @@ -1562,6 +1569,151 @@ class TestPreprocessor : public TestFixture {
ASSERT_EQUALS("asm ( )\n;\n\nbbb", actual.at(""));
}

void sdccAsmOperands() { // #6028
{
const char code[] = "void f() {\n"
"__asm\n"
" movx @dptr,a\n"
"__endasm;\n"
"}\n";
ASSERT_EQUALS("void f ( ) {\nasm ( )\n\n;\n}", getcode(settings0, *this, code).at(""));
}
{
const char code[] = "__asm\n"
" mov b,#(s_XINIT>>8)\n"
"__endasm";
ASSERT_EQUALS("asm ( )\n\n;", getcode(settings0, *this, code).at(""));
}
{
const char code[] = "int f(int x) {\n"
" ++x;\n"
" __asm\n"
" anl a,#0x0f\n"
" inc a\n"
" movc a,@a+pc\n"
" ret\n"
" __endasm;\n"
" return x;\n"
"}\n";
ASSERT_EQUALS("int f ( int x ) {\n++ x ;\nasm ( )\n\n\n\n\n;\nreturn x ;\n}",
getcode(settings0, *this, code).at(""));
}
ASSERT_EQUALS("", errout_str());
}

void sdccAsmEmptyAndAdjacent() {
ASSERT_EQUALS("asm ( )\n;", getcode(settings0, *this, "__asm\n__endasm").at(""));
const char code[] = "__asm\n"
"__endasm;\n"
"__asm\n"
" nop\n"
"__endasm;\n"
"int after;\n";
ASSERT_EQUALS("asm ( )\n;\nasm ( )\n\n;\nint after ;", getcode(settings0, *this, code).at(""));
ASSERT_EQUALS("", errout_str());
}

void sdccAsmComments() {
const char code[] = "/* __asm */\n"
"const char *s = \"__asm __endasm\";\n"
"__asm ; __endasm } #error ignored\n"
" ; __asm {\n"
" mov a,b ; __endasm\n"
" /* __endasm */ nop\n"
" mov a,__endasm\n"
"__endasm;\n"
"int after; // __asm\n";
ASSERT_EQUALS("\nconst char * s = \"__asm __endasm\" ;\nasm ( )\n\n\n\n\n;\nint after ;",
getcode(settings0, *this, code).at(""));
ASSERT_EQUALS("", errout_str());
}

void sdccAsmIncomplete() {
ASSERT_EQUALS("__asm\nnop", getcode(settings0, *this, "__asm\nnop").at(""));
ASSERT_EQUALS("__asm\nnop\n__asm\nnop", getcode(settings0, *this, "__asm\nnop\n__asm\nnop").at(""));
ASSERT_EQUALS("__asm\nnop\n__asm\nnop\n__endasm ;\nint after ;",
getcode(settings0, *this, "__asm\nnop\n__asm\nnop\n__endasm;\nint after;").at(""));
{
const char code[] = "void f() {\n"
"__asm\n"
" nop\n"
"}\n"
"int after;\n"
"__endasm;\n";
ASSERT_EQUALS("void f ( ) {\n__asm\nnop\n}\nint after ;\n__endasm ;",
getcode(settings0, *this, code).at(""));
}
{
const char code[] = "__asm\n"
" nop\n"
"#define N 1\n"
"__endasm;\n"
"int after;\n";
ASSERT_EQUALS("__asm\nnop\n\n__endasm ;\nint after ;", getcode(settings0, *this, code).at(""));
}
ASSERT_EQUALS("", errout_str());
}

void sdccAsmOtherSyntax() {
const char code[] = "void f() {\n"
" __asm (\"nop\");\n"
" __asm\n"
" (\"nop\");\n"
" __asm volatile (\"nop\");\n"
" __asm goto (\"\" : : : : label);\n"
"label: ;\n"
" __asm { nop }\n"
"}\n"
"__asm void g(void) {\n"
" nop\n"
"}\n";
ASSERT_EQUALS("void f ( ) {\n__asm ( \"nop\" ) ;\n__asm\n( \"nop\" ) ;\n"
"__asm volatile ( \"nop\" ) ;\n__asm goto ( \"\" : : : : label ) ;\n"
"label : ;\n__asm { nop }\n}\n__asm void g ( void ) {\nnop\n}",
getcode(settings0, *this, code).at(""));
ASSERT_EQUALS("\nvoid f ( ) { $__asm ( \"nop\" ) ; }",
getcode(settings0, *this, "#define ASM __asm\nvoid f() { ASM(\"nop\"); }").at(""));
ASSERT_EQUALS("", errout_str());
}

void sdccAsmConditional() {
const char code[] = "void f() {\n"
"#ifdef USE_ASM\n"
" __asm\n"
" mov b,#(s_XINIT>>8)\n"
" movx @dptr,a\n"
" __endasm;\n"
"#else\n"
" fallback();\n"
"#endif\n"
" after();\n"
"}\n";
ASSERT_EQUALS("void f ( ) {\n\nasm ( )\n\n\n;\n\n\n\nafter ( ) ;\n}",
getcodeforcfg(settings0, *this, code, "USE_ASM", "file.c"));
ASSERT_EQUALS("void f ( ) {\n\n\n\n\n\n\nfallback ( ) ;\n\nafter ( ) ;\n}",
getcodeforcfg(settings0, *this, code, "", "file.c"));
ASSERT_EQUALS("", errout_str());
}

void sdccAsmControlFlow() {
const char code[] = "void f(int x) {\n"
" if (x)\n"
" __asm\n"
" nop\n"
" __endasm;\n"
" else\n"
" fallback();\n"
" do\n"
" __asm\n"
" nop\n"
" __endasm /* comment */;\n"
" while (x);\n"
"}\n";
ASSERT_EQUALS("void f ( int x ) {\nif ( x )\nasm ( )\n\n;\nelse\nfallback ( ) ;\n"
"do\nasm ( )\n\n;\nwhile ( x ) ;\n}", getcode(settings0, *this, code).at(""));
ASSERT_EQUALS("", errout_str());
}

void endifsemicolon() {
const char filedata[] = "void f() {\n"
"#ifdef A\n"
Expand Down
Loading