Message ID | 20140812192204.GA13299@host2.jankratochvil.net |
---|---|
State | Committed |
Headers |
Received: (qmail 750 invoked by alias); 12 Aug 2014 19:22:15 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 732 invoked by uid 89); 12 Aug 2014 19:22:14 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 required=5.0 tests=AWL, BAYES_00, RP_MATCHES_RCVD, SPF_HELO_PASS, SPF_PASS, URIBL_BLACK autolearn=no version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Tue, 12 Aug 2014 19:22:13 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s7CJM801023313 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 12 Aug 2014 15:22:09 -0400 Received: from host2.jankratochvil.net (ovpn-116-19.ams2.redhat.com [10.36.116.19]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s7CJM4e1025766 (version=TLSv1/SSLv3 cipher=AES128-GCM-SHA256 bits=128 verify=NO); Tue, 12 Aug 2014 15:22:07 -0400 Date: Tue, 12 Aug 2014 21:22:04 +0200 From: Jan Kratochvil <jan.kratochvil@redhat.com> To: Yao Qi <yao@codesourcery.com> Cc: gdb-patches@sourceware.org Subject: [patch+7.8?] Fix --with-babeltrace with gcc-4.9.1 Message-ID: <20140812192204.GA13299@host2.jankratochvil.net> References: <20140804202907.GA2608@host2.jankratochvil.net> <53E17E14.8070104@codesourcery.com> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="ikeVEW9yuYc//A+q" Content-Disposition: inline In-Reply-To: <53E17E14.8070104@codesourcery.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-IsSubscribed: yes |
Commit Message
Jan Kratochvil
Aug. 12, 2014, 7:22 p.m. UTC
On Wed, 06 Aug 2014 03:00:04 +0200, Yao Qi wrote: > This patch fixes https://sourceware.org/bugzilla/show_bug.cgi?id=17104 > Please mention it in ChangeLog entry. I had a patch to this PR, but > didn't follow it up. Yours is fine. I think the one of yours is better, posting it here for new approval. IMO it could go also for 7.8.1. My original reproducer is wrong, one also has to specify -Wall in CFLAGS: CFLAGS=-Wall ./configure --with-babeltrace; make Thanks, Jan gdb/ 2014-07-01 Yao Qi <yao@codesourcery.com> PR build/17104 * configure.ac: Use local variable 'pos'. * configure: Regenerated.
Comments
On Tue, Aug 12, 2014 at 12:22 PM, Jan Kratochvil <jan.kratochvil@redhat.com> wrote: > On Wed, 06 Aug 2014 03:00:04 +0200, Yao Qi wrote: >> This patch fixes https://sourceware.org/bugzilla/show_bug.cgi?id=17104 >> Please mention it in ChangeLog entry. I had a patch to this PR, but >> didn't follow it up. Yours is fine. > > I think the one of yours is better, posting it here for new approval. > > IMO it could go also for 7.8.1. > > > My original reproducer is wrong, one also has to specify -Wall in CFLAGS: > CFLAGS=-Wall ./configure --with-babeltrace; make > > > Thanks, > Jan > > gdb/ > 2014-07-01 Yao Qi <yao@codesourcery.com> > > PR build/17104 > * configure.ac: Use local variable 'pos'. > * configure: Regenerated. > > diff --git a/gdb/configure b/gdb/configure > index a4c0a8c..7956aa7 100755 > --- a/gdb/configure > +++ b/gdb/configure > @@ -15234,6 +15234,7 @@ struct bt_iter_pos *pos = bt_iter_get_pos (bt_ctf_get_iter (NULL)); > struct bt_ctf_event *event = NULL; > const struct bt_definition *scope; > > + bt_iter_set_pos (bt_ctf_get_iter (NULL), pos); > scope = bt_ctf_get_top_level_scope (event, > BT_STREAM_EVENT_HEADER); > bt_ctf_get_uint64 (bt_ctf_get_field (event, scope, "id")); > diff --git a/gdb/configure.ac b/gdb/configure.ac > index a2ac15f..fc1d8bc 100644 > --- a/gdb/configure.ac > +++ b/gdb/configure.ac > @@ -2417,6 +2417,7 @@ else > struct bt_ctf_event *event = NULL; > const struct bt_definition *scope; > > + bt_iter_set_pos (bt_ctf_get_iter (NULL), pos); > scope = bt_ctf_get_top_level_scope (event, > BT_STREAM_EVENT_HEADER); > bt_ctf_get_uint64 (bt_ctf_get_field (event, scope, "id")); > Hi. This seems like an excessive amount of code just to test whether a library exists. Do we really need all of it? E.g., can we just delete "pos" and the function call that initializes it? struct bt_iter_pos *pos = bt_iter_get_pos (bt_ctf_get_iter (NULL)); Or, if for some reason we need to test whether bf_ctf_get_iter exists, can we just call it and discard the result? [And similarly for the rest of the code.] None of this code gets run anyways.
On Tue, 12 Aug 2014 22:32:55 +0200, Doug Evans wrote: > E.g., can we just delete "pos" and the function call that initializes it? > > struct bt_iter_pos *pos = bt_iter_get_pos (bt_ctf_get_iter (NULL)); I think we cannot and the reason is given in the comment there: # Append -Werror to CFLAGS so that configure can catch the warning # "assignment from incompatible pointer type", which is related to # the babeltrace change from 1.0.3 to 1.1.0. Babeltrace 1.1.0 works # in GDB, while babeltrace 1.0.3 is broken. > Or, if for some reason we need to test whether bf_ctf_get_iter exists, > can we just > call it and discard the result? Also not due to the reason above. Jan
On Tue, Aug 12, 2014 at 1:38 PM, Jan Kratochvil <jan.kratochvil@redhat.com> wrote: > On Tue, 12 Aug 2014 22:32:55 +0200, Doug Evans wrote: >> E.g., can we just delete "pos" and the function call that initializes it? >> >> struct bt_iter_pos *pos = bt_iter_get_pos (bt_ctf_get_iter (NULL)); > > I think we cannot and the reason is given in the comment there: > # Append -Werror to CFLAGS so that configure can catch the warning > # "assignment from incompatible pointer type", which is related to > # the babeltrace change from 1.0.3 to 1.1.0. Babeltrace 1.1.0 works > # in GDB, while babeltrace 1.0.3 is broken. > > >> Or, if for some reason we need to test whether bf_ctf_get_iter exists, >> can we just >> call it and discard the result? > > Also not due to the reason above. Alas the comment doesn't specify which assignment. I think it's for scope. I did some grepping: @ruffy:babeltrace$ find babeltrace-1.0.3 -name '*.h' | xargs grep bt_iter_get_pos babeltrace-1.0.3/include/babeltrace/iterator.h: * - restore is a position saved with bt_iter_get_pos, it is used with babeltrace-1.0.3/include/babeltrace/iterator.h: * bt_iter_get_pos - Get the current iterator position. babeltrace-1.0.3/include/babeltrace/iterator.h:struct bt_iter_pos *bt_iter_get_pos(struct bt_iter *iter); @ruffy:babeltrace$ find babeltrace-1.1.0 -name '*.h' | xargs grep bt_iter_get_pos babeltrace-1.1.0/include/babeltrace/iterator.h: * - restore is a position saved with bt_iter_get_pos, it is used with babeltrace-1.1.0/include/babeltrace/iterator.h: * bt_iter_get_pos - Get the current iterator position. babeltrace-1.1.0/include/babeltrace/iterator.h:struct bt_iter_pos *bt_iter_get_pos(struct bt_iter *iter); @ruffy:babeltrace$ find babeltrace-1.0.3 -name '*.h' | xargs grep bt_ctf_get_top_level_scope babeltrace-1.0.3/include/babeltrace/ctf/events.h: * bt_ctf_get_top_level_scope: return a definition of the top-level scope babeltrace-1.0.3/include/babeltrace/ctf/events.h:const struct definition *bt_ctf_get_top_level_scope(const struct bt_ctf_event *event, @ruffy:babeltrace$ find babeltrace-1.1.0 -name '*.h' | xargs grep bt_ctf_get_top_level_scope babeltrace-1.1.0/include/babeltrace/ctf/events.h: * bt_ctf_get_top_level_scope: return a definition of the top-level scope babeltrace-1.1.0/include/babeltrace/ctf/events.h:const struct bt_definition *bt_ctf_get_top_level_scope(const struct bt_ctf_event *event, Note that there's no change in the result of bt_iter_get_pos, but there is in bt_ctf_get_top_level_scope. Plus some digging found this: https://sourceware.org/ml/gdb-patches/2013-03/msg00955.html
On 08/13/2014 04:32 AM, Doug Evans wrote: > This seems like an excessive amount of code just to test whether a > library exists. > Do we really need all of it? IMO, it's better to keep them. When I use babeltrace in GDB, I find the babeltrace APIs are not stable, so I put more code in the configure test, to cover GDB usages. > E.g., can we just delete "pos" and the function call that initializes it? > > struct bt_iter_pos *pos = bt_iter_get_pos (bt_ctf_get_iter (NULL)); > > Or, if for some reason we need to test whether bf_ctf_get_iter exists, > can we just > call it and discard the result? [And similarly for the rest of the code.] > None of this code gets run anyways. As I said above, bt_iter_get_pos and bf_ctf_get_iter are here to test they still exist in the babeltrace library. They are in 1.1.0, but I am worried that they may be changed or renamed in the future.
On Tue, Aug 12, 2014 at 6:49 PM, Yao Qi <yao@codesourcery.com> wrote: > On 08/13/2014 04:32 AM, Doug Evans wrote: >> This seems like an excessive amount of code just to test whether a >> library exists. >> Do we really need all of it? > > IMO, it's better to keep them. When I use babeltrace in GDB, I find the > babeltrace APIs are not stable, so I put more code in the configure > test, to cover GDB usages. > >> E.g., can we just delete "pos" and the function call that initializes it? >> >> struct bt_iter_pos *pos = bt_iter_get_pos (bt_ctf_get_iter (NULL)); >> >> Or, if for some reason we need to test whether bf_ctf_get_iter exists, >> can we just >> call it and discard the result? [And similarly for the rest of the code.] >> None of this code gets run anyways. > > As I said above, bt_iter_get_pos and bf_ctf_get_iter are here to test > they still exist in the babeltrace library. They are in 1.1.0, but I am > worried that they may be changed or renamed in the future. Thanks. I suspect it'll be useful to be able to refer to this reasoning at some point in the future. [ref: the "incompatible pointer type" warning is related to the assignment to scope, not pos]. Maybe this thread is sufficient, or maybe you could add something to the commit message. Patch is ok with me.
diff --git a/gdb/configure b/gdb/configure index a4c0a8c..7956aa7 100755 --- a/gdb/configure +++ b/gdb/configure @@ -15234,6 +15234,7 @@ struct bt_iter_pos *pos = bt_iter_get_pos (bt_ctf_get_iter (NULL)); struct bt_ctf_event *event = NULL; const struct bt_definition *scope; + bt_iter_set_pos (bt_ctf_get_iter (NULL), pos); scope = bt_ctf_get_top_level_scope (event, BT_STREAM_EVENT_HEADER); bt_ctf_get_uint64 (bt_ctf_get_field (event, scope, "id")); diff --git a/gdb/configure.ac b/gdb/configure.ac index a2ac15f..fc1d8bc 100644 --- a/gdb/configure.ac +++ b/gdb/configure.ac @@ -2417,6 +2417,7 @@ else struct bt_ctf_event *event = NULL; const struct bt_definition *scope; + bt_iter_set_pos (bt_ctf_get_iter (NULL), pos); scope = bt_ctf_get_top_level_scope (event, BT_STREAM_EVENT_HEADER); bt_ctf_get_uint64 (bt_ctf_get_field (event, scope, "id"));