Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
From: Antoine Tremblay <antoine.tremblay@ericsson.com>
To: Yao Qi <qiyaoltc@gmail.com>, Simon Marchi <simon.marchi@ericsson.com>
Cc: <gdb-patches@sourceware.org>
Subject: Re: [PATCH] Replace some xmalloc-family functions with XNEW-family ones
Date: Tue, 18 Aug 2015 13:09:00 -0000	[thread overview]
Message-ID: <55D32E92.2070506@ericsson.com> (raw)
In-Reply-To: <86614d3rsu.fsf@gmail.com>



On 08/18/2015 04:49 AM, Yao Qi wrote:
> Simon Marchi <simon.marchi@ericsson.com> writes:
>
> Hi Simon,
> Thanks for doing this.
>
>> This patch is part of the make-gdb-buildable-in-C++ effort.  The idea is
>> to change some calls to the xmalloc family of functions to calls to the
>> equivalents in the XNEW family.  This avoids adding an explicit cast, so
>> it keeps the code a bit more readable.  Some of them also map relatively
>> well to a C++ equivalent (XNEW (struct foo) -> new foo), so it will be
>> possible to do scripted replacements if needed.
>
> I am not against this patch, and I think this patch is useful to shorten
> the code in some cases.  However, I don't think we really need it to
> make GDB buildable in C++, right?  We can still use xmalloc in a C++
> program (xmalloc is still in use in GCC).
>
> XNEW/xmalloc -> new or struct -> class conversion should be done per
> data structure rather than globally like this patch does.  We only
> convert C-inheritance data structures like breakpoint_ops, varobj, etc,
> to C++ class, and leave the rest there unless it is necessary to change
> them to C++ class.  This is my personal understanding, and I'd like to
> hear others thoughts.
>
>>
>> I only changed calls that were obviously allocating memory for one or
>> multiple "objects".  Allocation of variable sizes (such as strings or
>> buffer handling) will be for later (and won't use XNEW).
>>
>>    - xmalloc (sizeof (struct foo)) -> XNEW (struct foo)
>>    - xmalloc (num * sizeof (struct foo)) -> XNEWVEC (struct foo, num)
>>    - xcalloc (1, sizeof (struct foo)) -> XCNEW (struct foo)
>>    - xcalloc (num, sizeof (struct foo)) -> XCNEWVEC (struct foo, num)
>>    - xrealloc (p, num * sizeof (struct foo) -> XRESIZEVEC (struct foo, p, num)
>>    - obstack_alloc (ob, sizeof (struct foo)) -> XOBNEW (ob, struct foo)
>>    - obstack_alloc (ob, num * sizeof (struct foo)) -> XOBNEWVEC (ob, struct foo, num)
>>    - alloca (sizeof (struct foo)) -> XALLOCA (struct foo)
>>    - alloca (num * sizeof (struct foo)) -> XALLOCAVEC (struct foo, num)
>>
>> Some instances of xmalloc followed by memset to zero the buffer were
>> replaced by XCNEW or XCNEWVEC.
>
> I am not against this conversion.  If we accept this change, are
> xmalloc/xcalloc/... not allowed to use, and XNEW/XNEWVEC/... should be
> used instead.  The restriction is not necessary to me.
>
>>
>> I regtested on x86-64, Ubuntu 14.04, but the patch touches many
>> architecture-specific files.  For those I'll have to rely on the
>> buildbot or people complaining that I broke their gdb.
>
> If arch-specific files are changes, we have several ways to make sure
> changes don't break build at least,
>
>   - Configure gdb with --enable-targets=all --enable-64-bit-bfd and
>     build,  all *-tdep.c changes can be covered,
>   - Download some cross compile, such as arm/aarch64/mips gcc, and cross
>     compile native gdb for these archs, for example, configure gdb with
>     --host=arm-linux-gnueabihf --target=arm-linux-gnueabihf CC=/foo/bar/arm-linux-gnueabihf-gcc
>   - Install mingw32 toolchain, and cross compile native gdb for win32.
>
> You can also apply for gcc compile farm account to build patched GDB
> there.  The complains of buildbot or people are the last guards, and we
> should trigger as less complains as we can.
>

Just a note , I have to test my build also for multi arch soon and I 
checked the gcc compile farm, while it looked very promising the current 
online servers are ppc and x86 only...

Cross compiling looks like a better way , I plan to use 
https://github.com/palves/multi-build for inspiration to make a makefile 
for all this...

Unfortunately though with cross compiling the tests won't be done..



  parent reply	other threads:[~2015-08-18 13:09 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-08-17 21:53 Simon Marchi
2015-08-18  8:49 ` Yao Qi
2015-08-18 11:13   ` Pedro Alves
2015-08-18 12:56     ` Yao Qi
2015-08-18 13:09   ` Antoine Tremblay [this message]
2015-08-19 15:31   ` Simon Marchi
2015-08-26 19:24     ` Simon Marchi
2015-08-26 19:44       ` Pedro Alves
2015-08-26 21:20         ` Simon Marchi
2015-08-26 21:33           ` Simon Marchi
2015-08-27  9:54     ` Yao Qi
2015-08-27 16:56       ` DJ Delorie
2015-08-27 17:48 Doug Evans

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=55D32E92.2070506@ericsson.com \
    --to=antoine.tremblay@ericsson.com \
    --cc=gdb-patches@sourceware.org \
    --cc=qiyaoltc@gmail.com \
    --cc=simon.marchi@ericsson.com \
    /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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox