patchSimulavr: an AVR simulator - Patches: patch #7640, Fixes error in branch instruction...


You are not allowed to post comments on this tracker with your current authentification level.

patch #7640: Fixes error in branch instruction trace: bogus target labels

Submitted by:  Sebas Vila-Marta <sebas>
Submitted on:  Sun Oct 23 12:08:19 2011  
Category: bug fixPriority: 5 - Normal
Status: DonePrivacy: Public
Assigned to: NoneOpen/Closed: Closed
Component Version: simulavr

Mon May 20 15:35:26 2013, comment #4:

Ups :-)

I did a fix also todo before looking arround all the patches and bugs here.

The problem is solved!


Klaus Rudolph <zfrdh>
Project Administrator
Tue Nov 22 16:54:31 2011, comment #3:

Hi Petr,

I have a modest knowledge of simulavr architecture... so my opinion should be taken with a grain of salt ;-)

Following your comments I think :

a) AVR architecture has two banks of memory. Let's name them flash and ram. The minimum addressable unit in ram are bytes. The minimum addressable unit in flash are words (16b). This is true despite of the instructions like LPM. These instructions can be better seen as instructions fetching a word and then selecting the low or hight byte.

b) According to the last item, it seems natural in simulavr to address ram using byte-based addresses and flash using word-based addresses. I see no need to change it. Then I would vote for option (A) of your proposal.

c) (This is a little off-topic) In my opinion it's much more artificial to consider PC as a byte-based address. PC always holds flash addresses, thus it should be word-based. Considering PC word-based would make a lot of expressions like "PC*2" spreaded along the code to rewrite as a simpler "PC".

Sebas Vila-Marta <sebas>
Fri Nov 18 23:26:43 2011, comment #2:

(I'm sorry for the delay.)
Ok, I see the bug and your patch would definitely fix it.

Trace() of some instructions use byte-based addresses, some use word-based. AvrDevice::Load() fills the symbol array using byte-based addresses for symbols in RAM and word-based for symbols in Flash (see calls to AddSymbol).

Most places in simulavr use "address" to mean a byte-based value; "index" or "offset" for word-based values - similar to terminology used by GDB. I fix places when the naming is swapped.
It would be nice if the code used only one way to specify a memory location, probably byte-based way. But I do not know if it is worth converting.

These functions use the word-based addressing: avr_op_JMP::Trace(), avr_op_LPM_Z::Trace(), avr_op_LPM::Trace(), avr_op_LPM_Z_incr::Trace()

We should either:
(A) use accept your patch (which turns avr_op_BRBC and avr_op_BRBS to word-based symbol lookup) and rename the variables to `index' or
(B) turn the above mentioned functions to byte-based lookup and not rename

Fellows, should we do A or B?

Petr Hluzin <petrh>
Project Member
Tue Oct 25 10:24:27 2011, comment #1:

Forgot to submit the patch :-/ ... Now it sould be ok.

(file #24194)

Sebas Vila-Marta <sebas>
Sun Oct 23 12:08:19 2011, original submission:
Sebas Vila-Marta <sebas>


Attached Files
file #24194:  simulavr-1.patch added by sebas (2kB - text/x-patch)


Depends on the following items: None found

Items that depend on this one: None found


Carbon-Copy List
  • -unavailable- added by zfrdh (Posted a comment)
  • -unavailable- added by petrh (Posted a comment)
  • -unavailable- added by sebas (Submitted the item)

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.


    Please enter the title of George Orwell's famous dystopian book (it's a date):



    Follow 3 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Mon May 20 15:35:26 2013zfrdhStatusNone=>Done
    Tue Oct 25 10:24:27 2011sebasAttached File-=>Added simulavr-1.patch, #24194

    Back to the top

    Powered by Savane 3.1-cleanup1