All of lore.kernel.org
 help / color / mirror / Atom feed
From: Arnout Vandecappelle <arnout@mind.be>
To: buildroot@busybox.net
Subject: [Buildroot] [PATCH] gdb: convert to the package infrastructure
Date: Fri, 01 Mar 2013 22:42:41 +0100	[thread overview]
Message-ID: <513120D1.1000201@mind.be> (raw)
In-Reply-To: <20130228095645.2cdfffd6@skate>


>>    Just a couple of generic remarks, and then a few specific ones
>> below. The generic remarks are probably for follow-up patches.
>>
>> * It really doesn't make sense to build host-gdb without a gdbserver
>> for the target. So I would auto-select gdbserver from host-gdb.
>
> See the point made by James Hogan regarding kgdb. I don't have a strong
> opinion on this one, and we could decide that gdbserver is lightweight
> enough that even with James use case of kgdb, it still makes sense to
> have gdbserver on the target when host-gdb is enabled.

  Running a remote kgdb is something you would need the most on a system 
with little memory, so indeed the non-gdbserver use case is important.
So, keep it as you have it.


>> * Does it really make sense to keep options for four different gdb
>> versions? Can't we just remove the user-selectable version completely?
>
> I just kept as it was.

  As I said: for a follow-up patch.

  James Hogan makes another good point that custom patches against a 
specific gdb version are easier to maintain if the version is selectable. 
However, when the generic package infrastructure is used, changing the 
version number directly in gdb.mk is also easy. I think the use case of 
custom patches against gdb is sufficiently rare that Kconfig 
configuration is not needed.


>> * Do gdb-7.2+ actually work for avr32? If yes, why keep the 6.7.1
>> around?
>
> I also just kept it as it was. If we want to upgrade gdb for avr32,
> then let's do it as a follow-up. People having access to avr32 hardware
> should test that.

  Actually, I mean: there is an avr32-specific gdb version, but the 7.x 
gdb versions are also enabled for avr32. It just looks strange.


>>> ---
>>> It would be good to have a few people testing this before
>>> committing.
>>
>>    Nah, just let the autobuilders do it :-)
>
> Autobuilders are nice, but they are not yet capable of starting
> gdbserver on the target, connect gdb, and verify that it works :)

  True, but the change you make here is not going to affect gdb runtime. 
The likelihood that there is serious build breakage is much higher than 
that of a runtime regression, I think. Also if there is a runtime 
regression, the chance that it will be found is much higher if it's been 
committed than if the patch is just lingering on the list.


>>> +	  For embedded development, the most common solution is to
>>> +	  build only 'gdbserver' for the target, and use a
>>> cross-gdb
>>> +	  on the host. See BR2_PACKAGE_HOST_GDB to enable one.
>>
>>    "See BR2_PACKAGE_HOST_GDB in the Toolchain menu to enable one."
>>
>>    It could be useful to add a comment that external toolchains often
>> provide gdb/gdbserver.
>
> Indeed. I am not sure what do with external toolchains here. In the
> past, I was hiding the option to build host-gdb and gdbserver for
> the target, making the assumption that the external toolchain already
> have those, and instead there was the option to copy the toolchain's
> gdbserver to the target. Now, everything is available: host-gdb built
> by Buildroot, gdbserver built by Buildroot, and gdbserver copied from
> the external toolchain. Should I keep all this?

  You could make the gdbserver copied from the external toolchain 
mutually exclusive with the gdb package, but I'm not convinced that that 
is needed.


> Also, the "gdbserver build by Buildroot" thing is in Packages ->
> Debugging, but the "copy gdbserver from toolchain" thing is in
> Toolchain. But it's not easy to put it in a package, as the code needed
> to copy gdbserver from the toolchain really relates to all the complex
> external toolchain logic.

  Well, the location of the config menu is independent from where it is 
used in the .mk files, of course.

  Wild idea:

In Toolchain menu, under all the rest:

  [ ] Cross gdb for the host
      GDB debugger version (gdb 7.5.x)  -->
  [ ] gdbserver on the target
  [ ]   Copy external toolchain gdbserver to the target
  [ ] full gdb on the target


  Hm, this looks very much like the current situation :-)

  Admittedly, gdb's Config.in would look pretty non-standard (e.g. a 
blind BR2_PACKAGE_GDB symbol that is selected by 
BR2_PACKAGE_GDB_GDBSERVER and BR2_PACKAGE_GDB_DEBUGGER), but it is a 
fairly special case.

[snip]
>>    Can we rely on host-nurses being present? It is not checked in
>> dependencies.sh AFAIK. Admittedly, it is not a dependency of host-gdb
>> now, but most people have ncurses installed anyway for menuconfig...
>
> host-ncurses wasn't a dependency, and our host-ncurses package is only
> here to build a version of "tic" that works properly. Some systems had
> a "tic" version causing problems when building a more recent ncurses
> for the target. I think that other than for this "tic" program, we
> should really on the pre-installed ncurses library, which should
> already be present since the user uses menuconfig.

  I guess even better would be a build-if-missing approach like for tar 
and python (when the patch gets merged).

>
> However, it's true we don't check it in dependencies.sh, and the user
> could very well be using xconfig or gconfig, neither of which require
> ncurses.
>
> I'd say that's a separate matter really.

  Agreed. This patch is just an opportunity to review gdb in general.

>
>> [snip]
>>> +define GDB_REMOVE_UNNEEDED_FILES
>>> +	$(RM) -rf $(TARGET_DIR)/usr/share/gdb
>>> +endef
>>
>>    Perhaps a comment why this is needed? Or is it just to keep it
>> consistent with the current behaviour?
>
> The old gdb installation logic was to manually copy the gdb binary to
> the $(TARGET_DIR). In my patch, I've chosen to use 'make install'
> instead, which requires removing a bunch of generally useless stuff
> (Python scripts and XML descriptions of targets, which as per my
> understanding, are needed only when you do remote debugging).

  Fair enough. I feel something like this is worthwhile to explain in a 
comment.


  Regards,
  Arnout
-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F

      reply	other threads:[~2013-03-01 21:42 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-02-26 22:13 [Buildroot] [PATCH] gdb: convert to the package infrastructure Thomas Petazzoni
2013-02-28  8:00 ` Arnout Vandecappelle
2013-02-28  8:39   ` James Hogan
2013-02-28  8:47     ` Thomas Petazzoni
2013-02-28 12:11       ` James Hogan
2013-02-28 12:20         ` Thomas Petazzoni
2013-02-28 12:24           ` James Hogan
2013-02-28  8:56   ` Thomas Petazzoni
2013-03-01 21:42     ` Arnout Vandecappelle [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=513120D1.1000201@mind.be \
    --to=arnout@mind.be \
    --cc=buildroot@busybox.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.