Braindead Assembly Code



  • This from T Chan. I know aboslutely nothing about assembly ... but since these never show up, I thought I'd atleast sidebar this one .... maybe you'll get it.

    So I'm doing a group project. Every group member has his/her faults, of course, but I like to think mine (terminal laziness) is not as bad as some of the others (choosing to do the ARM assembly code, with little experience or competence or sense).

    Naturally, since assembly code is horrible to maintain, you'd structure it as well as possible. Or maybe not.

    The first obvious problem (if you read from the top) is he overwrites the stack pointer (r13) and the link register (r14). He overwrites the stack pointer right after setting it, too. (other problems is he uses r2 and r3 without realising it'll probably be overwritten by function calls).
        ldr        sp,=0x7FFF        ; Set the stack position to the end of our memory
        BL        UART_INIT        ; Initialize the UART
        ;Get the address of the start of DPRAM into r13.
        ;This is DPRAM address 0, where the signals from the FPGA are read from.
        LDR r13,=EXC_DPSRAM_BLOCK0_BASE
        ;Add the constant 4 to the base address of the ram to address DPRAM word 1.
       ;The LSB of this register controls the LED.
        ADD r14, r13, #4

    The main control flow is spaghetti code (worse, composeSubFrames branches either to MAINLOOP or RP2), with little documentation of what register holds what (comments are removed). The label RP4 is mysteriously missing, so I don't know how he can claim that he thinks it works when it can't compile.

    MAINLOOP
        B getNextSubFrame
    RP1
        B composeSubFrames
    RP2
        B stripAwayC
    RP3
            B HAMMING2
            B HAMMING3
    RP5
        B extractBits
    RP6
        B checkParity
    RP7
        B handleStates
    RP8
        B outputData
    RP9
        B MAINLOOP

    Some strange code to load 0x101 (you can MOV r1,#0x100):
        MOV r1, #255
        ADD r1, r1, #2

    Shift right-left-right to do bit masking
        MOV r1, r1, LSR #8
        MOV r1, r1, LSL #31
        MOV r1, r1, LSR #23

    not-quite-functions that don't do very much
    stripAwayC
        ;strip away C from r2
        MOV r2, r2, LSL #23
        MOV r2, r2, LSR #23
        ;strip away C from r3
        MOV r3, r3, LSL #23
        MOV r3, r3, LSR #23
        B RP3

    And the crown.... What most people would implement as an array lookup, he implements as a giant switch statement in assembly!
    HAMMING2
    ;Correct values:
    ;Obtained from: 00000
    ;9-bit code 000000000
       MOV r1, #0
       ADD r1, r1, #0
       CMP r1, r2
       MOVEQ r2, #0
       BEQ RP4
    ....
    ;9-bit code 111111111
       MOV r1, #255
       ADD r1, r1, #255
       ADD r1, r1, #1
       CMP r1, r2
       MOVEQ r2, #31
       BEQ RP4
    ;Other error values:
        B hammingError
    ;Total number of codes: 320

    And because he didn't code proper functions, to do the same thing to a different register requires, you guessed it...
    HAMMING3
    ;Correct values:
    ;Obtained from: 00000
    ;9-bit code 000000000
       MOV r1, #0
       ADD r1, r1, #0
       CMP r1, r3
       MOVEQ r3, #0
       BEQ RP5
    ...

    The killer is that all that code was auto-generated with a Java program (I suppose at least he didn't do it all by hand).

    He says he spent 40 hours on it. It's 4846 lines, so I can sort-of see why (even though the non-auto-generated code only goes to line 211)

    My nearly complete rewrite only weighs in at 250 lines.
     



  • Regarding setting the SP then overwriting it, presumably there's no way to tell in advance whether UART_INIT is going to make further jumps, nor how many levels deep these jumps will be. My ARM is fuzzy but I think it will need to save the intermediate link registers on the stack if it does so, because the CPU never stacks the return address for you. So it needs a bit of stack somewhere. I take it that 7FFF is the top of memory, which seems like a good place to start the stack, if you don't know how large it will become.

    And regarding the "crazy" MOV, can you shift the immediate values? I don't remember, but the reference sheet implies not. But whatever, you only get 8 (contiguous) bits; so MOV Rn,#0x100 (aka #1 LSL #8, assuming you get the shiftings) buys you nowt, because you still need to add in the 1. Surely you can't get 0x101 in 1 instruction! 255+2 is perfectly fine; an alternative might be MOV Rn,#1 then OR Rn,Rn LSL #8.

    Disclaimer: was too young to afford an Acorn Archimedes, so only the 6502 coding has stuck. And I only replied because the epitome of terminal laziness is surely getting the computer to write the code out for you, and not bothering to optimize it, rather than bumming the code senselessly down to some teeny tiny number of lines.

    I don't doubt, of course, that this particular solution really does suck very very badly indeed, and that the poster's solution was a significant improvement :)



  • What's with the editing time limit? I am incorrect regarding the link register, because getNextSubFrame (etc.) appear to be externally-provided function calls (as I assume UART_INIT is too). Sorry.



  • Oh, geez.  How to totally hose your program in 2 instructions or less!

    Here's my own one: I once spent weeks and weeks poring over code to track down a problem with a device.  It would run for a while, and then hardlock.  Here's the really crazy part: I put some debug tracing in, and did

    func_call_here();
    if (err != OS_NO_ERR) {
      sprintf(msg, "error: %s", err, error_namse[err]);
      display_string(msg);
      while(1); // stop things forever
    }

    Sometimes the device would lock up saying "error: OS_NO_ERR". Usually under high serial load. After much grief, I tracked it down to this code in the task-switcher (the OS port to our processor was done by someone else, and I think they just downloaded most of it from the web):

        msr     SPSR_c, r4           ; spsr = saved spsr

    This restored the PSR (program status register, basically CPU control bits, permission level, instruction set type, and status flags) of the switched-to thread. Now, I had looked at this code before, but didn't understand it -- that instruction's syntax is not in the ARM4 instruction set document, nor is the raw binary value it encodes to.  After a long while, I finally determined what it meant: Load the SPSR register with the value from r4 -- but only the control bits! The flags part, specifically (which is how a CPU stores the results of comparisons, i.e. this < that, this == that, this == 0) were NOT restored.  Now, the code that did a voluntary thread switch (current thread is waiting for a signal) didn't care about the flags.  But if an interrupt caused a thread to be suspended in order to switch to a higher-priority thread (such as a serial port interrupt waking up the thread talking to the serial port), the SPSR would keep the flags from thread #2 when restoring thread #1. So if the interrupt happened at a particular point in time:
     
      cmp $r0, #0   ; is err == OS_NO_ERR (0) ? (yes, it is: Z=1)
    ; * serial port interrupt activates serial port thread,
    ; * upon resumption, now Z=0                  
      beq no_error  ; nope, no error, skip the next code (Z=0, doesn't skip)
      bl display_error_message
    no_error:

    The solution was insanely simple:

        msr     SPSR_cxsf, r4           ; restore all four pieces of the SPSR.



  • @Tom_ said:

    Regarding setting the SP then overwriting it, presumably there's no way to tell in advance whether UART_INIT is going to make further jumps, nor how many levels deep these jumps will be. My ARM is fuzzy but I think it will need to save the intermediate link registers on the stack if it does so, because the CPU never stacks the return address for you. So it needs a bit of stack somewhere. I take it that 7FFF is the top of memory, which seems like a good place to start the stack, if you don't know how large it will become.

    And regarding the "crazy" MOV, can you shift the immediate values? I don't remember, but the reference sheet implies not. But whatever, you only get 8 (contiguous) bits; so MOV Rn,#0x100 (aka #1 LSL #8, assuming you get the shiftings) buys you nowt, because you still need to add in the 1. Surely you can't get 0x101 in 1 instruction! 255+2 is perfectly fine; an alternative might be MOV Rn,#1 then OR Rn,Rn LSL #8.

    Disclaimer: was too young to afford an Acorn Archimedes, so only the 6502 coding has stuck. And I only replied because the epitome of terminal laziness is surely getting the computer to write the code out for you, and not bothering to optimize it, rather than bumming the code senselessly down to some teeny tiny number of lines.

    I don't doubt, of course, that this particular solution really does suck very very badly indeed, and that the poster's solution was a significant improvement :)



    Here's the problem: There's no place (well, there are a few places if he's faithfully following ATPCS, but I'm betting he isn't) that you can safely store the original SP without risk of UART_INIT trashing it.

    Futhermore, if anybody else had already begun using 0x7FFF as stack space, then anything he stores on the stack will totally hose the original data there.




  • > The killer is that all that code was auto-generated with a Java program (I suppose at least he didn't do it all by hand).

    Well, this explains everything. I can imaging asking one of my fellow
    Java-programmers to write some assembler code. He will dig Internet
    for meanings of assembler instructions (or better download and
    print
    some manual), and later write something like this. Not his fault

    • there's some gap between assembler and Java, really.



  •  One of things I hate most is people who use different names to point to the same register. If your going to use sp, fine, but don't use r13 lower down, same with lr and r14 and pc/r15. It drives me mad. 



  • "Arise Sir Thread." I'd put an image in as well but I can't be arsed.



    Any particular reason for rezzing a 4 year convo?



  • @PJH said:

    Any particular reason for rezzing a 4 year convo?
     

    Probably a spamrez and then an inadvertent response.



  • @dhromed said:

    @PJH said:

    Any particular reason for rezzing a 4 year convo?
     

    Probably a spamrez and then an inadvertent response.

    Correct.


Log in to reply
 

Looks like your connection to What the Daily WTF? was lost, please wait while we try to reconnect.