ASM: Invalid absolute jmp/call address

Daniel Green avatarDaniel Green created an issue

When attempting to execute a jmp/call from an array, GDC incorrectly generates the address.

extern (C) void*[5] ptr;

void foo() {}

extern (C) void main()
{
	ptr[1] = &foo;
	asm 
	{
		jmp ptr[1*4];
	}
}

// invalid jump statement produced by GDC
jmp 4+*ptr(%rip)

// Correct syntax
jmp *ptr(%rip)+4 // The entire address needs a * prefix

Reordering the if block at line 2180 of d-asm-i386.h to the end of the case statement produced correct results. However, I'm not sure if it will produce any other side effects.

Comments (12)

  1. Iain Buclaw

    Need to take register displacement into consideration.

    @@ -2177,14 +2177,11 @@
                             writeReg(operand->segmentPrefix);
                             insnTemplate->writebyte(':');
                         }
    -                    if ((operand->segmentPrefix != Reg_Invalid && operand->symbolDisplacement.dim == 0) ||
    +                    // Register displacement, this writes out 'x' in x[REG]
    +                    if ((operand->segmentPrefix != Reg_Invalid && operand->symbolDisplacement.dim == 0) &&
                             operand->constDisplacement)
                         {
                             addOperand("%a", Arg_Integer, newIntExp(operand->constDisplacement), asmcode);
    -                        if (operand->symbolDisplacement.dim)
    -                        {
    -                            insnTemplate->writebyte('+');
    -                        }
                             operand->constDisplacement = 0;
                             if (opInfo->operands[i] & Opr_Dest)
                                 asmcode->clobbersMemory = 1;
    @@ -2305,6 +2302,16 @@
                                     addOperand(fmt, Arg_Memory, e, asmcode, mode);
                                 }
                             }
    +                        // Symbol displacement, this writes out 'x' in [SYM]+x
    +                        if (operand->constDisplacement)
    +                        {
    +                            insnTemplate->writebyte('+');
    +                            addOperand("%a", Arg_Integer, newIntExp(operand->constDisplacement), asmcode);
    +                            operand->constDisplacement = 0;
    +
    +                            if (opInfo->operands[i] & Opr_Dest)
    +                                asmcode->clobbersMemory = 1;
    +                        }
                         }
                         if (use_star)
                             insnTemplate->writebyte('*');
    

    Test that first. Check for any side effects, then commit.

    Thanks.

  2. Daniel Green

    The * is used for absolute addressing, so I moved it to the top of the code block. It appeared to me that the if statement that handles symbolDisplacement determines on it's own when to use a *. With that in mind, I made them exclusive to each other and applying your changes came up with the following.

    diff -r 584a5f3a7dce d/d-asm-i386.h
    --- a/d/d-asm-i386.h	Fri Apr 29 14:13:02 2011 +0100
    +++ b/d/d-asm-i386.h	Mon Jun 20 11:12:31 2011 -0400
    @@ -2186,20 +2186,19 @@
                             mode = Mode_Input;
     
                         use_star = opTakesLabel();//opInfo->takesLabel();
    +                    if (use_star && operand->symbolDisplacement.dim == 0)
    +                        insnTemplate->writebyte('*');
    +                    
     
                         if (operand->segmentPrefix != Reg_Invalid)
                         {
                             writeReg(operand->segmentPrefix);
                             insnTemplate->writebyte(':');
                         }
    -                    if ((operand->segmentPrefix != Reg_Invalid && operand->symbolDisplacement.dim == 0) ||
    +                    if ((operand->segmentPrefix != Reg_Invalid && operand->symbolDisplacement.dim == 0) &&
                             operand->constDisplacement)
                         {
                             addOperand("%a", Arg_Integer, newIntExp(operand->constDisplacement), asmcode);
    -                        if (operand->symbolDisplacement.dim)
    -                        {
    -                            insnTemplate->writebyte('+');
    -                        }
                             operand->constDisplacement = 0;
                             if (opInfo->operands[i] & Opr_Dest)
                                 asmcode->clobbersMemory = 1;
    @@ -2321,8 +2320,16 @@
                                 }
                             }
                         }
    -                    if (use_star)
    -                        insnTemplate->writebyte('*');
    +                    if (operand->constDisplacement)
    +                    {
    +                        if (operand->symbolDisplacement.dim)
    +                            insnTemplate->writebyte('+');
    +                        addOperand("%a", Arg_Integer, newIntExp(operand->constDisplacement), asmcode);
    +                        operand->constDisplacement = 0;
    +                        if (opInfo->operands[i] & Opr_Dest)
    +                            asmcode->clobbersMemory = 1;                     
    +                    }
    +                    
                         if (operand->baseReg != Reg_Invalid || operand->indexReg != Reg_Invalid)
                         {
                             insnTemplate->writebyte('(');
    

    For 32-bit code it appears to work correctly, 64-bit code has an issue with function calls attempting to use RIP relative addressing which neither GCC or GDC does on for C/D code. I believe it's a separate issue.

    foo(); // call    _D3asm3fooFZv
    asm
    {
        call foo; // call _D3asm3fooFZv(%rip)
        call ptr[1*4];
    }
    

    Working test case.

    // gdc -g asmcall.d -m32
    extern (C) void*[5] ptr;
    extern (C) int printf(char*,...);
    
    void foo()
    {
            printf(cast(char*)"foo\n");
    }
    
    
    void main(char[][] args)
    {
            ptr[1] = &foo;
            foo();
            asm
            {
                    call foo;
                    call ptr[1*4];
            }
    }
    

    If you think it looks good then I can go and commit it.

  3. Daniel Green

    The problem I ran in to was at line 2320 of d-asm-i386.h is that depending on the type of variable and OS, the displacement needs to be before or after the memory reference.

    call *%fs:ptr@tpoff+4 // linux TLS access.  displacement can only come after.
    call *4+-64(%rbp) // linux local variable.  displacement cannot come after (%rbp)
    

    Windows is roughly the same, the difference is that emulated TLS uses register access instead of segment offset.

  4. Iain Buclaw

    going off the above:

    @@ -2177,14 +2229,9 @@
                             writeReg(operand->segmentPrefix);
                             insnTemplate->writebyte(':');
                         }
    -                    if ((operand->segmentPrefix != Reg_Invalid && operand->symbolDisplacement.dim == 0) ||
    -                        operand->constDisplacement)
    +                    if (operand->segmentPrefix != Reg_Invalid && operand->symbolDisplacement.dim == 0)
                         {
                             addOperand("%a", Arg_Integer, newIntExp(operand->constDisplacement), asmcode);
    -                        if (operand->symbolDisplacement.dim)
    -                        {
    -                            insnTemplate->writebyte('+');
    -                        }
                             operand->constDisplacement = 0;
                             if (opInfo->operands[i] & Opr_Dest)
                                 asmcode->clobbersMemory = 1;
    @@ -2297,12 +2344,32 @@
                                 }
                                 else
                                 {
    +                                Expression * offset = newIntExp(operand->constDisplacement);
    +
                                     if (use_star)
                                     {
                                         insnTemplate->writebyte('*');
                                         use_star = false;
                                     }
    -                                addOperand(fmt, Arg_Memory, e, asmcode, mode);
    +
    +                                if (decl->isDataseg())
    +                                {   // Displacement can only come after symbol
    +                                    addOperand(fmt, Arg_Memory, e, asmcode, mode);
    +                                    insnTemplate->writebyte('+');
    +                                    addOperand("%a", Arg_Integer, offset, asmcode);
    +                                }
    +                                else
    +                                {   // Displacement cannot come after symbol.
    +                                    addOperand("%a", Arg_Integer, offset, asmcode);
    +                                    addOperand(fmt, Arg_Memory, e, asmcode, mode);
    +                                }
    +                            }
    +                            if (operand->constDisplacement)
    +                            {   // If a memory reference was displaced, tell GCC to not keep
    +                                // memory values cached in registers across the instruction.
    +                                if (opInfo->operands[i] & Opr_Dest)
    +                                    asmcode->clobbersMemory = 1;
    +                                operand->constDisplacement = 0;
                                 }
                             }
                         }
    
    

    Fix as needed, attaching patch for the gcc-4.6 branch - which has some other things thrown in with it.

  5. Daniel Green

    The offset is being output with a 0 prepended causing gas to treat it as an octal.

     # 205 "c:\mingw\msys\1.0\home\venix\gdc-official\dev\gcc-4.5.2\libphobos\core\atomic.d" 1
    	movl 016(%ebp), %edx
     # 0 "" 2
    	.loc 2 206 0
     # 206 "c:\mingw\msys\1.0\home\venix\gdc-official\dev\gcc-4.5.2\libphobos\core\atomic.d" 1
    	movl 012(%ebp), %eax
     # 0 "" 2
    	.loc 2 207 0
     # 207 "c:\mingw\msys\1.0\home\venix\gdc-official\dev\gcc-4.5.2\libphobos\core\atomic.d" 1
    	movl 08(%ebp), %ecx
    
  6. Iain Buclaw
    {   // Displacement cannot come after symbol.
        addOperand("%a", Arg_Integer, offset, asmcode);
        if (decl->isParameter())
        {   // Parameter may have offset that will add to this value.
            insnTemplate->writebyte('+');
        }
        addOperand(fmt, Arg_Memory, e, asmcode, mode);
    }
    

    see e01697578501

  7. Daniel Green

    Testing the patch, via transplant, on the default branch still produces incorrect memory access on MinGW.

    // other is a local variable.
    // ptr is TLS
    
    	call ptr+4; //call *(%eax)+4 .  I think this would be correct with linux, since TLS uses segment prefix.  However, for MinGW it must be call *4+(%eax).
    	call other+4; //call *428(%esp).  I think 428 should probably be 4+28(%esp)
    

    Test case.

    extern (C) void*[5] ptr;
    extern (C) int printf(char*,...);
    
    __gshared void*[5] old;
    
    void foo() 
    {
    	printf(cast(char*)"foo\n");
    }
    
    
    void main(char[][] args)
    {
    	void*[5] other;
    	other[1] = &foo;
    	old[1] = &foo;
    	ptr[1] = &foo;
    	foo();
    	asm 
    	{
    		call foo;
    		call ptr[1*4];
    		call old+4;
    		call ptr+4;
    		call other+4;
    		
    	}
    }
    

    Edit: GAS doesn't complain about compiling the assembly file, however call %(ebp)+4 still fails. Correcting the errors mentioned earlier to what I felt was the proper notion allowed proper execution.

  8. Daniel Green

    Here are the changes made to your patch to fix the errors mentioned earlier. It still needs testing on Linux but look it over and see if anything should be changed.

    diff -r 9b0d7c8816cf d/d-asm-i386.h
    --- a/d/d-asm-i386.h	Sat Jul 09 23:47:10 2011 +0100
    +++ b/d/d-asm-i386.h	Sat Jul 16 22:08:11 2011 -0400
    @@ -2319,17 +2319,24 @@
                                     {
                                         Expression * offset = newIntExp(operand->constDisplacement);
     
    -                                    if (decl->isDataseg())
    +                                    if ((decl->isDataseg() && !decl->isThreadlocal()) 
    +                                        || (decl->isThreadlocal() && targetm.have_tls))
                                         {   // Displacement can only come after symbol
    +                                        // If all TLS implementations don't use the segment
    +                                        // register a preprocessor directive may be required.
                                             addOperand(fmt, Arg_Memory, e, asmcode, mode);
                                             insnTemplate->writebyte('+');
                                             addOperand("%a", Arg_Integer, offset, asmcode);
                                         }
                                         else
                                         {   // Displacement cannot come after symbol.
    +                                        // Emulated TLS has same requirement.
                                             addOperand("%a", Arg_Integer, offset, asmcode);
    -                                        if (decl->isParameter())
    +                                        if (decl->isParameter()
    +                                            || decl->toParent2()->isFuncDeclaration() != NULL
    +                                            || decl->isThreadlocal())
                                             {   // Parameter may have offset that will add to this value.
    +                                            // As will local variables and emulated TLS.
                                                 insnTemplate->writebyte('+');
                                             }
                                             addOperand(fmt, Arg_Memory, e, asmcode, mode);
    
    
  9. Daniel Green

    Wrong patch got uploaded.

    Edit: Linux is has issues with global variables using RIP addressing still. That is the __gshared variable in the test file. It doesn't put a '+' between the offset and displacment resulting in 4-64(%rip). Only occurs with 64-bit.

    Edit: Mingw gives warning about missing operand with thread local variables. So the decl->isThreadlocal() will probably need to be removed from the '+' test.

  10. Log in to comment
Tip: Filter by directory path e.g. /media app.js to search for public/media/app.js.
Tip: Use camelCasing e.g. ProjME to search for ProjectModifiedEvent.java.
Tip: Filter by extension type e.g. /repo .js to search for all .js files in the /repo directory.
Tip: Separate your search with spaces e.g. /ssh pom.xml to search for src/ssh/pom.xml.
Tip: Use ↑ and ↓ arrow keys to navigate and return to view the file.
Tip: You can also navigate files with Ctrl+j (next) and Ctrl+k (previous) and view the file with Ctrl+o.
Tip: You can also navigate files with Alt+j (next) and Alt+k (previous) and view the file with Alt+o.