Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expand the I/O table to support the ATmega2560 #416

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

edgar-bonet
Copy link

I had simavr segfault on me by running sts 0x01ff, r0 on a simulated ATmega2560. This turned out to be _avr_set_r() calling an invalid function pointer found past the end of the array avr->io.

This pull request fixes the issue by expanding the avr->io array in order to cover the full I/O space of the ATmega2560.

The I/O space of the ATmega640/1280/1281/2560/2561 extends from 0x0020
to 0x01ff, for a total of 512 - 32 = 480 memory locations.

Fixes a crash that would result from writing to an unmapped location.
@buserror
Copy link
Owner

Hmm are you sure about that? AVR use 16 bits addresses, and I ran a LOT of code on 256's without that problem showing at all. That code has been there for a LONG time I should think this would have been picked up by now

@edgar-bonet
Copy link
Author

Hmm are you sure about that?

Absolutely. I can reliably segfault simavr with this:

echo -e "sts 0x01ff, r0\nsleep" > crash.s
avr-gcc -mmcu=atmega2560 -nostdlib crash.s -o crash.elf
./run_avr -f 1000000 -m atmega2560 crash.elf

That code has been there for a LONG time I should think this would have been picked up by now

The thing is, 0x01ff is not a valid I/O port: it's one of those “reserved” locations at the top of the I/O space. No legit AVR program will expose this bug. A buggy program, however, could poke into one of these reserved locations. Such program may crash, but it should not crash the simulator itself.

@buserror
Copy link
Owner

buserror commented Apr 1, 2021

In that case I'd really like some sort of error message to signal the problem. I'd MUCH, MUCH prefer simavr crashing because of an out of bounds problem than simavr silently working, and then have a random crash on the hardware I can't debug because "it works in simavr"!
Ultimately, simavr is suposed to be a tool that let you develop/debug your firmware.

@edgar-bonet
Copy link
Author

In that case I'd really like some sort of error message to signal the problem.

Indeed, it would be really nice if simavr could print a warning whenever the firmware attempts to access a reserved memory location.

I'd MUCH, MUCH prefer simavr crashing because of an out of bounds problem than simavr silently working, and then have a random crash on the hardware I can't debug because "it works in simavr"!

I wouldn't. The error message “Segmentation fault” is a very clear indication that the crash is due to a bug in the simulator, not a bug in the firmware. I lost a lot of time figuring out that the simulator bug was triggered by a firmware bug.

Besides, even if we say segfaulting is an expected behavior (in which case it should be documented), the current behavior of simavr in this respect is inconsistent:

  • Simavr seems to emulate most reserved memory locations as regular R/W memory: writes are harmless, and reads return the last value written.

  • A few of these locations make simavr crash.

I just checked the behavior of an actual AVR chip. It's a mega328P, as I don't have a 2560 handy. I read and wrote all the reserved locations between the last I/O register and RAMSTART: 0xc7 through 0xff. From this experiment it seems that:

  • Writes are harmless and ignored.

  • All reads return 0, save from 0xc7 which returns 0x20.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants