Please help me check ...

[James Ladd]

All,

Below is a routine that I am using that is part of the code for a thread pool.
The code is in C and I am converting it by hand to assembler.
I'm doing this by hand because this way I get to 'know' the routine intimately.

What I am interested in are you comments on this conversion. It runs without
error, and I think it is correct, but I am not as advanced as some here with assembler hence the post
asking if you think this is correct.

Rgs, James.

Structures - C

Code Select Expand


typedef struct node {
  unsigned value;
  pointer_t next;
  unsigned foo[30];
} node_t;

typedef struct shared_mem {
  pointer_t head;
  unsigned foo1[31];
  pointer_t tail;
  unsigned foo2[31];
  node_t nodes[MAX_NODES+1];
  unsigned serial;
} shared_mem_t;


Structures - Assembler

Code Select Expand


# Define the queue node structure
#
.struct 0
NODE_START:
NODE.value:
    .struct NODE.value + 4
NODE.next:
    .struct NODE.next + 4
NODE.foo:
    .struct NODE.foo + (30 * 4)
NODE_END:
NODE_SIZE = NODE_END - NODE_START

# Define the thread pool structure
#
.struct 0
TPOOL_START:
TPOOL.jlclib:
    .struct TPOOL.jlclib + 4
TPOOL.head:
    .struct TPOOL.head + 4
TPOOL.foo1:
    .struct TPOOL.foo1 + (31 * 4)
TPOOL.tail:
    .struct TPOOL.tail + 4
TPOOL.foo2:
    .struct TPOOL.foo2 + (31 * 4)
TPOOL.nodes:
    .struct TPOOL.nodes + ((MAX_NODES +1) * NODE_SIZE)
TPOOL.serial:
    .struct TPOOL.serial + 4
TPOOL_END:
TPOOL_SIZE = TPOOL_END - TPOOL_START


Initialisation - C

Code Select Expand


void
init_queue()
{
  unsigned i;

  /* initialize queue */
  smp->head.sep.ptr = 1;
  smp->head.sep.count = 0;
  smp->tail.sep.ptr = 1;
  smp->tail.sep.count = 0;
  smp->nodes[1].next.sep.ptr = NULL;
  smp->nodes[1].next.sep.count = 0;

  /* initialize avail list */
  for (i=2; i<MAX_NODES; i++) {
    smp->nodes[i].next.sep.ptr = i+1;
    smp->nodes[i].next.sep.count = 0;
  }
  smp->nodes[MAX_NODES].next.sep.ptr = NULL;
  smp->nodes[MAX_NODES].next.sep.count = 0;

  /* initialize queue contents */
  if (initial_nodes > 0) {
    for (i=2; i<initial_nodes+2; i++) {
      smp->nodes[i].value = i;
      smp->nodes[i-1].next.sep.ptr = i;
      smp->nodes[i].next.sep.ptr = NULL;
    }
    smp->head.sep.ptr = 1;
    smp->tail.sep.ptr = 1 + initial_nodes;
  }
}


Initialisation - Assembler

Code Select Expand


_jlc_tpool_open:
        push ebp
        mov ebp, esp

        # Params are:
        # jlclib [ebp +8], tpool* [ebp +12], tcount [ebp +16], qcount [ebp +20]
        # TODO: Make use of qcount as we ignore it right now.

        # No local variables (yet).

        # Steps:
        # 1. Allocate the thread pool structure and,
        # 2. Initialize the thread pool structure and,
        # 3. Create the required threads waiting on a 'run' barrier and,
        # 4. Return the pointer to the thread pool [ebp +12].

        # 1.
        # for now I use a static piece of memory until we get the API
        # working. TODO: Make a call to a allocator for this.
        mov edx, offset temp_tpool

        # 4.
        mov ecx, [ebp +12]             # store in tpool* for result/later
        mov [ecx], edx

        mov ecx, [ebp +8]              # store jlclib handle into structure.
        mov [edx +TPOOL.jlclib], ecx

        # 2.
        mov [edx +TPOOL.head], dword ptr ((1 shl 16) + 0)
        mov [edx +TPOOL.tail], dword ptr ((1 shl 16) + 0)
        add edx, TPOOL.nodes + NODE_SIZE    # nodes[1]
        mov [edx +NODE.next], dword ptr 0

        mov ecx, dword ptr 1
        initialize_avail_list:
            inc ecx

            mov eax, ecx
            inc eax
            shl eax, 16

            mov [edx +NODE.next], eax
            add edx, NODE_SIZE

            cmp ecx, MAX_NODES
            jne initialize_avail_list

        xor eax, eax
        mov [edx +NODE.next], eax

        # I don't initiaize queue contents.

        # 3. TODO: complete this part.

        xor eax, eax

        mov esp, ebp
        pop ebp
        ret


[James Ladd]

awwe, no comments.  :(

[Tedd]

No news is good news? :P

Looks okay from here, if a little long-winded (WRT the structure definition). Though I've never really played with GAS, so I'm no help :lol

[gabor]

Hello!

Don't give up so easily!  :U

Okay, let's see:
First of all I had some problem with the readability. Are you using MASM or other assembler? Honestly I am not familiarr with this kind of structure declaration...

For my own satisfaction I transformed your structures:

Code Select Expand


pointer_t       STRUCT

               count   dw ?
               _ptr    dw ?                            ; '_' symbol appended, because ptr is a reserved word in masm

pointer_t       ENDS

node_t          STRUCT

               dwValue dd ?
               next    pointer_t ?
               bFoo    db 32 dup (?)

node_t          ENDS

shared_mem_t    STRUCT

               head    pointer_t ?
               tail    pointer_t ?
               bFoo1   db 32 dup (?)
               bFoo2   db 32 dup (?)
               nodes   node_t MAX_NODES+1 dup (?)
               bSerial db ?
                       db ?,?,?                        ; padding

shared_mem_t    ENDS


I suggested that the unsigned refers to unsigned char that is a byte. I added 3 padding bytes in the shared_mem_t structure to align the structure to dwords.
I don't know the exact structure of pointer_t, but it seems it has a substructure with 2 fields: counter and ptr. As I could see in your code the pointer_t structure is always used with its substructure, so I guess it could be merged into the main structure. That's why I did not declare a seperate structure...

Now, the code:

Code Select Expand


_jlc_tpool_open PROC jlclib:DWORD,tpool:DWORD,tcount:DWORD,qcount:DWORD

               ; #1
               mov     edx,offset temp_tpool           ; would this be the tpool parameter later?
               ASSUME edx:PTR shared_mem_t

; #4 Return the pointer to the thread pool [ebp +12].
; I use ecx and eax 'interlaced' to avoid register stalls

               mov     eax,tpool                       ; store in tpool* for result/later
               mov     ecx,jlclib
               mov     [eax],edx                       ; store jlclib handle into structure.
               mov     [edx].jlclib,ecx

; #2 initialize queue
;  smp->head.sep.ptr = 1;
;  smp->head.sep.count = 0;
               mov     [edx].head,DWORD PTR 00010000h  ; =(1 shl 16) + 0
;  smp->tail.sep.ptr = 1;
;  smp->tail.sep.count = 0;
               mov     [edx].tail,DWORD PTR 00010000h
;  smp->nodes[1].next.sep.ptr = NULL;
;  smp->nodes[1].next.sep.count = 0;
;               add edx, TPOOL.nodes + NODE_SIZE    # nodes[1]
;               mov [edx +NODE.next], dword ptr 0
; I think this is not correct, or are you skipping nodes[0] intentionally?
; It may look like this:
               mov     [edx].nodes.next,DWORD PTR 0

; initialize avail list
               mov     ecx,1
               mov     eax,00010000h                   ; eax=ecx shl 16
               push    edx
;  for (i=2; i<MAX_NODES; i++) {
@@:
               add     eax,00010000h                   ; ptr = ecx+1
                add     edx,sizeof node_t               ; start with second node
                add     ecx,1
;    smp->nodes[i].next.sep.ptr = i+1;
;    smp->nodes[i].next.sep.count = 0;
               mov     [edx].nodes.next,eax
               cmp     ecx,MAX_NODES
               jc @B
;  }

;  smp->nodes[MAX_NODES].next.sep.ptr = NULL;
;  smp->nodes[MAX_NODES].next.sep.count = 0;
               mov     [edx+sizeof node_t].nodes.next,DWORD PTR 0
               pop     edx

               ret

_jlc_tpool_open ENDP


I hope I did not mess up too much in there and my post proves to be usefull!

Greets, Gábor

[James Ladd]

Guys thanks for the reply.

Im not sure why you (Gabor) wanted to convert from GAS to MASM but thanks.

I guess the big thing is neither of you said there were obvious errors so Im rather happy about that.

[gabor]

Hi!

Sorry, I was a slow learner... I didn't realize that you were talking about GAS ( I don't eve know what that is  :( )

However, your implementation looks good, some optimizations could be done there. But it should work!  :U

Greets, Gábor

[James Ladd]

Gabor, what optimisations do you think can be done?

[tenkey]

Just a note:

unsigned is short for unsigned int