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 authentication level.

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

Submitted by:  Sebas Vila-Marta <sebas>
Submitted on:  Sun 23 Oct 2011 12:08:19 PM UTC  
Category:  bug fix Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  None Open/Closed:  Closed
Component Version:  simulavr

Mon 20 May 2013 03:35:26 PM UTC, 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 22 Nov 2011 04:54:31 PM UTC, 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 18 Nov 2011 11:26:43 PM UTC, 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 25 Oct 2011 10:24:27 AM UTC, comment #1:

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

(file #24194)

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


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


Depends on the following items: None found

Items that depend on this one: None found


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

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

    Only logged-in users can vote.




    Follow 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2013-05-20 zfrdh StatusNone => Done
        Open/ClosedOpen => Closed
    2011-10-25 sebas Attached File- => Added simulavr-1.patch, #24194

    Back to the top

    Powered by Savane 3.3