Bug 13519 - [avr][Regression 7.1/7.2/7.3/7.4] disassemble command attempts to disassemble SRAM rather than program memory
Summary: [avr][Regression 7.1/7.2/7.3/7.4] disassemble command attempts to disassemble...
Status: NEW
Alias: None
Product: gdb
Classification: Unclassified
Component: gdb (show other bugs)
Version: 7.7
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-19 12:44 UTC by Joerg Wunsch
Modified: 2017-04-04 18:47 UTC (History)
8 users (show)

See Also:
Host:
Target: avr-*-*
Build:
Last reconfirmed:


Attachments
The patch (947 bytes, patch)
2012-02-24 11:27 UTC, Michael N. Lipp
Details | Diff
patch against gdb-7.7.1 (887 bytes, patch)
2014-07-08 19:51 UTC, cristiano.dealti
Details | Diff
Final patch to broken AVR arch with the latest gdbs (243 bytes, patch)
2015-05-24 17:40 UTC, cristiano.dealti
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joerg Wunsch 2011-12-19 12:44:03 UTC
The dissassemble command now attempts to disassemble SRAM locations
rather than program memory:

% ./gdb-7.2/gdb/gdb -q foo.elf
Reading symbols from /simtemp/jw/foo.elf...done.

(gdb) disas 0x1, 0x3
Dump of assembler code from 0x800001 to 0x800003:
   0x00800001:  Cannot access memory at address 0x800001

(Due to the AVR's Harvard architecture, SRAM is remapped internally in
the GNU tools to an offset of 0x800000 which is beyond the addressable
program memory area of an AVR.  In contrast, program memory starts at
offset 0.)

Previous versions of GDB didn't experience this problem:

% ./gdb-7.0.1/gdb/gdb -q foo.elf
Reading symbols from /simtemp/jw/foo.elf...done.
(gdb) disas 1 3
Dump of assembler code from 0x1 to 0x3:
0x00000001 <__vectors+1>:       sbci    r25, 0x64       ; 100
End of assembler dump.

The difference is that, obviously, the start and end locations are now
being treated as expressions rather than numbers (which also caused
the requirement to separate them by a comma rather than a space).
Somehow, during evaluation of the expression, the offset 0x800000 gets
added.
Comment 1 Joerg Wunsch 2011-12-19 13:36:13 UTC
Some more analysis:

disassemble_command() calls value_as_address() on the first argument
(and eventually parse_and_eval_address() on the second one).

This function has a lengthy comment about Harvard-architecture address
mapping and its side effects.  It then calls
gdbarch_integer_to_address(), from there avr_integer_to_address(),
which eventually calls avr_make_saddr().  This functions adds
AVR_SMEM_START to everything except 0.

Something in that sequence is not quite right ;-), but I can't decide
what.
Comment 2 Eric Weddington 2011-12-19 14:55:19 UTC
Note: The user reporting this problem stated that the bug showed up originally in version 7.1:
http://lists.nongnu.org/archive/html/avr-chat/2011-12/msg00007.html
Comment 3 Michael N. Lipp 2012-02-24 11:27:25 UTC
Created attachment 6243 [details]
The patch
Comment 4 Michael N. Lipp 2012-02-24 11:28:04 UTC
The question is: how clever should gdb be. If you say

(gdb) disas (void (*)())0x1,(void (*)())0x3

it works. But of course, you'd expect gdb to do that cast automatically in the context of a disassemble.

I have attached a patch that does that cast for disassemble, both from the command line as well as for mi. gdb is rathr complicated. I hope my approach is correct.

Actually, the automatic cast should also be applied if you "x/i addr". But I didn't bother to look that up as I'm using gdb from Eclipse anyway.
Comment 5 Michael N. Lipp 2012-02-24 11:31:00 UTC
Sorry, forgot: the patch is against the 7.4 branch as of today.
Comment 6 Tom Tromey 2012-02-27 19:33:45 UTC
The best way to get a patch in is to follow the contribution
instructions: http://sourceware.org/gdb/contribute/
Comment 7 Eric Weddington 2012-03-09 16:00:59 UTC
(In reply to comment #5)
> Sorry, forgot: the patch is against the 7.4 branch as of today.

Hi Michael,

Do you have a FSF Copyright Assignment on file? You'll need that in order to contribute a non-trivial patch.

Thanks,
Eric Weddington
Comment 8 Dennis Tokarski 2014-05-25 21:32:26 UTC
Hi all,

Sorry for reviving a bug that's over two years old, but as
far as I can tell this problem is well understood yet has
never been fixed.

I ran into this a couple of weeks ago using avr-gdb-7.1 on
Fedora 18. More recently I've rebuilt the package using the
source from gdb 7.7 and the problem is still there.

Michael Lipp's patch is readily adapted to 7.7 and works, at
least in the situations he intended.

This issue was raised again on the gdb mailing list by
petr.hluzin@gmail.com in August of 2012. Eric seemed to
stop the thread short by stating "Adding 0x800000 to all
code-space addresses has been part of AVR GCC for many years",
which is not correct. I expect he meant to say "sram addresses"?

Regardless, is there some issue blocking inclusion of Michael's
patch? If it's a question of the maintainers wanting a different
approach for the sake of "doing it right", I'll be happy to work
something up given a little guidance.

Disassembly seems like a fairly major feature to leave broken
for this long even if AVR is kind of a corner use case.
Comment 9 Pedro Alves 2014-05-29 15:12:01 UTC
Someone needs to champion the patch upstream.  Please see:

 http://sourceware.org/gdb/contribute/

and:

 https://sourceware.org/gdb/wiki/ContributionChecklist
Comment 10 Dennis Tokarski 2014-05-30 18:40:23 UTC
Thanks for the links, Pedro, those are helpful.

I'll see what I can put together. It will be a few days.
Comment 11 cristiano.dealti 2014-07-07 18:17:16 UTC
Of course the bug is still present in the latest gdb 7.7.1 release.
It is also exacerbated when avr-gdb is used in Eclipse Juno because its Disassembly CDT plugin specifies source and end addresses in the -data-disassemble gdb command.

By enabling logging, it can be seen that gdb adds the 0x80000 offset to the requests to the avarice backend which reads data from sRAM instead from flash.

The response returned to gdb from avarice contains addresses with the 0x80000 offset (and wrong disassembled instructions of course). These responses are then rejected, due to address mismatch in the response, by the Eclipse DCT Disassembly plugin which retries with different parameters and eventually gives up.

All this significantly slows down debugging from Eclipse and makes the entire experience unpleasant.

Indeed, this two years old bug should be fixed. It cannot be that difficult.
Otherwise one has to spend a whole day figuring out what's going on, find the bug report and adapt the attached patch to the latest release. 

Dennis, did you succeed submitting the patch or something?
Comment 12 cristiano.dealti 2014-07-08 19:51:59 UTC
Created attachment 7692 [details]
patch against gdb-7.7.1

Adaptation of the previous patch to gdb 7.7.1
Comment 13 cristiano.dealti 2015-05-24 17:40:58 UTC
Created attachment 8330 [details]
Final patch to broken AVR arch with the latest gdbs

This patch obsoletes all previous patches attached to this bug.
The patch is trivial and only affects avr-tdep.c.
As noted by others, avr_integer_to_address() always returns addresses in SRAM.
However, the second parameter of this function, struct type *type, allows to understand if a code or data address is desired.

Indeed, patch is so simple that the patched function is simply:

static CORE_ADDR
avr_integer_to_address (struct gdbarch *gdbarch,
			struct type *type, const gdb_byte *buf)
{
  ULONGEST addr = unpack_long (type, buf);

  if (TYPE_DATA_SPACE (type))
    return avr_make_saddr (addr);
  else
    return avr_make_iaddr (addr);
}

Everything works: disassemble, breakpoints at numeric address, breakpoints at source line and watchpoints.
Comment 14 duckythescientist 2016-03-10 18:31:42 UTC
This bug still exists in the current release (7.11) and is causing problems. Building from source with patch in the comments corrects the problem. Why is this 4+ year old bugfix not yet merged?
Comment 15 Pedro Alves 2016-03-10 19:20:10 UTC
See comment #9.
Comment 16 Michael N. Lipp 2016-03-11 08:24:14 UTC
There you see why it is fun to contribute to most github projects and a nuisance to contribute to FSF projects. I've applied that patch to all gdb versions since 7.7 by now and am still very confident that it took me less time than creating that silly patch submission.

All the information is in this bug report in this (most upstream afaik) bug tracker. Use it. If we developed software with FSF procedures in our company, instead of using issue tracking we'd be bankrupt for a long time -- not only because of the effort, but also because we'd have lost all customers.
Comment 17 cristiano.dealti 2016-03-11 23:28:37 UTC
(In reply to Pedro Alves from comment #15)
> See comment #9.

Should I submit the patch to gdb-patches?
In that case should I mention this bug number?

Regards,
  Cristiano
Comment 18 Pedro Alves 2016-03-15 16:08:21 UTC
Yes please.  Please include a concise and self-contained rationale along with the patch, so it can added to the git log.
Comment 19 cristiano.dealti 2016-03-19 16:16:54 UTC
(In reply to Pedro Alves from comment #18)
> Yes please.  Please include a concise and self-contained rationale along
> with the patch, so it can added to the git log.

OK, done

https://sourceware.org/ml/gdb-patches/2016-03/msg00318.html

Regards,
  Cristiano
Comment 20 Eyck Jentzsch 2017-04-04 18:47:21 UTC
Hi,
the submission of the patch to gdb-patches is more than a year old. Is there any estimate when this patch will become part of the main line?