Skip to content

fix Issue 24130 - ImportC: Windows headers use inline asm with differ… #15595

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

Jump to bottom
Merged
merged 1 commit into from
Sep 11, 2023
Merged

fix Issue 24130 - ImportC: Windows headers use inline asm with differ… #15595

merged 1 commit into from
Sep 11, 2023

Conversation

WalterBright
Copy link
Member

…ent syntax

Microsoft inline asm looks like this:

    __asm    {
        mov     ecx, ShiftCount
        mov     eax, dword ptr [Value]
        mov     edx, dword ptr [Value+4]
        shrd    eax, edx, cl
        shr     edx, cl
    }

Look ma, no semicolons. ImportC's inline asm wants semicolons. Microsoft uses generic C code instead when _M_CEE_PURE is set, so we set it, too. The other macros Microsoft uses for generic C code look rather more dangerous to use:

#if defined(MIDL_PASS) || defined(RC_INVOKED) || defined(_M_CEE_PURE) \
    || defined(_68K_) || defined(_MPPC_) \
    || defined(_M_IA64) || defined(_M_AMD64) || defined(_M_ARM) || defined(_M_ARM64) \
    || defined(_M_HYBRID_X86_ARM64)

@WalterBright WalterBright added Review:Easy Review Feature:ImportC Pertaining to ImportC support labels Sep 9, 2023
@dlang-bot
Copy link
Contributor

Thanks for your pull request, @WalterBright!

Bugzilla references

Auto-close Bugzilla Severity Description
✓ 24130 critical ImportC: Windows headers use inline asm with different syntax

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

To target stable perform these two steps:

  1. Rebase your branch to upstream/stable:
git rebase --onto upstream/stable upstream/master
  1. Change the base branch of your PR to stable

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#15595"

@WalterBright
Copy link
Member Author

Now I get:

C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.29.30133\include\vadefs.h(70): Error: no type-specifier for declarator

Of course, I don't have that particular vadefs.h so I don't know what is on line 70. Sigh. But evidently, _M_CEE_PURE is going down some nutburger path. Not sure what to do here.

@WalterBright
Copy link
Member Author

Decided to try parsing Microsoft asm syntax.

@WalterBright WalterBright force-pushed the fix24130 branch 3 times, most recently from a189282 to a8057c9 Compare September 10, 2023 07:39
@WalterBright
Copy link
Member Author

Hmm, how do I disable the omf platform from running?

@WalterBright
Copy link
Member Author

how do I disable the omf platform from running?

The test runner apparently cannot do this. But I found another way...

@WalterBright WalterBright merged commit 2e66b7e into dlang:master Sep 11, 2023
@WalterBright WalterBright deleted the fix24130 branch September 11, 2023 01:26
ibuclaw
ibuclaw requested changes Sep 11, 2023
View reviewed changes
Copy link
Member

@ibuclaw ibuclaw left a comment

Choose a reason for hiding this comment

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

See comments, I see regressions and breaking downstream compilers.

compiler/src/dmd/cparse.d
@@ -626,7 +626,7 @@ final class CParser(AST) : Parser!AST

default:
// ImportC extensions: parse as a D asm block.
s = parseAsm();
s = parseAsm(compileEnv.masm);
Copy link
Member

Choose a reason for hiding this comment

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

I'd say just pass true instead of relying on...

compiler/src/dmd/dmodule.d
@@ -780,7 +780,9 @@ extern (C++) final class Module : Package
{
filetype = FileType.c;

global.compileEnv.masm = target.os == Target.OS.Windows && !target.omfobj; // Microsoft inline assembler format
Copy link
Member

Choose a reason for hiding this comment

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

... platform-specific global variables that gdc (and ldc?) don't define or set.

compiler/src/dmd/parse.d
Comment on lines +7039 to 7043
case TOK.endOfLine:
nextDefineLine();
goto case;

case TOK.semicolon:
Copy link
Member

Choose a reason for hiding this comment

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

This looks like it unintentionally bleeds into D code parser. Did you forget an if (endOfLine) { ... }?

compiler/src/dmd/parse.d
case TOK.semicolon:
if (nestlevel != 0)
error("mismatched number of curly brackets");

if (toklist || label)
{
// Create AsmStatement from list of tokens we've saved
AST.Statement s = new AST.AsmStatement(token.loc, toklist);
AST.AsmStatement as = new AST.AsmStatement(token.loc, toklist);
as.caseSensitive = !endOfLine;
Copy link
Member

Choose a reason for hiding this comment

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

These two variables mean different things, why is one setting the other?

@just-harry just-harry mentioned this pull request Apr 22, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ibuclaw ibuclaw ibuclaw requested changes

@thewilsonator thewilsonator thewilsonator approved these changes

Assignees
No one assigned
Labels
Feature:ImportC Pertaining to ImportC support Review:Easy Review Severity:Bug Fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@WalterBright @dlang-bot @ibuclaw @thewilsonator