[patch] Fix --with-babeltrace with gcc-4.9.1

[patch] Fix --with-babeltrace with gcc-4.9.1

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 1653 bytes --]

Hi,

when I tried to use --with-babeltrace on Fedora Rawhide x86_64
using gcc-4.9.1-3.fc22.x86_64 I got:

checking for libbabeltrace... no
configure: error: babeltrace is missing or unusable
Makefile:7973: recipe for target 'configure-gdb' failed

configure:15890: checking for libbabeltrace
configure:15918: gcc -o conftest -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches  -m64 -mtune=generic   -Werror   -static-libstdc++ -static-libgcc -Wl,-z,relro    conftest.c -lselinux -lncurses -lz -lm  -ldl   /usr/lib64/libbabeltrace.so /usr/lib64/libbabeltrace-ctf.so >&5
conftest.c: In function 'main':
conftest.c:198:21: error: unused variable 'pos' [-Werror=unused-variable]
 struct bt_iter_pos *pos = bt_iter_get_pos (bt_ctf_get_iter (NULL));
                     ^
cc1: all warnings being treated as errors
configure:15918: $? = 1
configure: failed program was:

The patch below fixes it for me.

In configure.ac there is above this check:
  # 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.
  # AC_LIB_HAVE_LINKFLAGS may modify CPPFLAGS in it, so it should be
  # safe to save and restore CFLAGS here.
  saved_CFLAGS=$CFLAGS
  CFLAGS="$CFLAGS -Werror"

Maybe it would be easier to use there:
  CFLAGS="$CFLAGS -Werror -Wno-unused-variable"

But maybe -Werror is cross-compiler compatible while -Wno-unused-variable is
not, I have no idea.


Jan

[-- Attachment #2: gdb-babeltrace-configure.patch --]
[-- Type: text/plain, Size: 1084 bytes --]

gdb/
2014-08-04  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* configure.ac (--with-babeltrace): Use 'pos'.
	* configure: Regenerate.

diff --git a/gdb/configure.ac b/gdb/configure.ac
index 70d0964..07d2f00 100644
--- a/gdb/configure.ac
+++ b/gdb/configure.ac
@@ -2437,6 +2437,7 @@ else
 			struct bt_ctf_event *event = NULL;
 			const struct bt_definition *scope;
 
+			(void) pos; /* Prevent -Werror=unused-variable.  */
 			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 b/gdb/configure
index 809326a..b983d16 100755
--- a/gdb/configure
+++ b/gdb/configure
@@ -15344,6 +15344,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;
 
+			(void) pos; /* Prevent -Werror=unused-variable.  */
 			scope = bt_ctf_get_top_level_scope (event,
 			      				   BT_STREAM_EVENT_HEADER);
 		        bt_ctf_get_uint64 (bt_ctf_get_field (event, scope, "id"));

Re: [patch] Fix --with-babeltrace with gcc-4.9.1

[Doug Evans]

On Mon, Aug 4, 2014 at 1:29 PM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> Hi,
>
> when I tried to use --with-babeltrace on Fedora Rawhide x86_64
> using gcc-4.9.1-3.fc22.x86_64 I got:
>
> checking for libbabeltrace... no
> configure: error: babeltrace is missing or unusable
> Makefile:7973: recipe for target 'configure-gdb' failed
>
> configure:15890: checking for libbabeltrace
> configure:15918: gcc -o conftest -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches  -m64 -mtune=generic   -Werror   -static-libstdc++ -static-libgcc -Wl,-z,relro    conftest.c -lselinux -lncurses -lz -lm  -ldl   /usr/lib64/libbabeltrace.so /usr/lib64/libbabeltrace-ctf.so >&5
> conftest.c: In function 'main':
> conftest.c:198:21: error: unused variable 'pos' [-Werror=unused-variable]
>  struct bt_iter_pos *pos = bt_iter_get_pos (bt_ctf_get_iter (NULL));
>                      ^
> cc1: all warnings being treated as errors
> configure:15918: $? = 1
> configure: failed program was:
>
> The patch below fixes it for me.
>
> In configure.ac there is above this check:
>   # 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.
>   # AC_LIB_HAVE_LINKFLAGS may modify CPPFLAGS in it, so it should be
>   # safe to save and restore CFLAGS here.
>   saved_CFLAGS=$CFLAGS
>   CFLAGS="$CFLAGS -Werror"
>
> Maybe it would be easier to use there:
>   CFLAGS="$CFLAGS -Werror -Wno-unused-variable"
>
> But maybe -Werror is cross-compiler compatible while -Wno-unused-variable is
> not, I have no idea.
>
>
> Jan
>
> gdb/
> 2014-08-04  Jan Kratochvil  <jan.kratochvil@redhat.com>
>
>         * configure.ac (--with-babeltrace): Use 'pos'.
>         * configure: Regenerate.
>
> diff --git a/gdb/configure.ac b/gdb/configure.ac
> index 70d0964..07d2f00 100644
> --- a/gdb/configure.ac
> +++ b/gdb/configure.ac
> @@ -2437,6 +2437,7 @@ else
>                         struct bt_ctf_event *event = NULL;
>                         const struct bt_definition *scope;
>
> +                       (void) pos; /* Prevent -Werror=unused-variable.  */
>                         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 b/gdb/configure
> index 809326a..b983d16 100755
> --- a/gdb/configure
> +++ b/gdb/configure
> @@ -15344,6 +15344,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;
>
> +                       (void) pos; /* Prevent -Werror=unused-variable.  */
>                         scope = bt_ctf_get_top_level_scope (event,
>                                                            BT_STREAM_EVENT_HEADER);
>                         bt_ctf_get_uint64 (bt_ctf_get_field (event, scope, "id"));
>

I don't know enough about babeltrace to say whether we can just remove
pos altogether.
I'm happy with the patch as is though.

Re: [patch] Fix --with-babeltrace with gcc-4.9.1

[Yao Qi]

On 08/05/2014 04:29 AM, Jan Kratochvil wrote:
> gdb/
> 2014-08-04  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* configure.ac (--with-babeltrace): Use 'pos'.
> 	* configure: Regenerate.
> 
> diff --git a/gdb/configure.ac b/gdb/configure.ac
> index 70d0964..07d2f00 100644
> --- a/gdb/configure.ac
> +++ b/gdb/configure.ac
> @@ -2437,6 +2437,7 @@ else
>  			struct bt_ctf_event *event = NULL;
>  			const struct bt_definition *scope;
>  
> +			(void) pos; /* Prevent -Werror=unused-variable.  */

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.

-- 
Yao (齐尧)

[patch+7.8?] Fix --with-babeltrace with gcc-4.9.1

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 477 bytes --]

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

[-- Attachment #2: 1 --]
[-- Type: text/plain, Size: 1073 bytes --]

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"));

Re: [patch+7.8?] Fix --with-babeltrace with gcc-4.9.1

[Doug Evans]

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.

Re: [patch+7.8?] Fix --with-babeltrace with gcc-4.9.1

[Jan Kratochvil]

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

Re: [patch+7.8?] Fix --with-babeltrace with gcc-4.9.1

[Doug Evans]

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

Re: [patch+7.8?] Fix --with-babeltrace with gcc-4.9.1

[Yao Qi]

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.

-- 
Yao (齐尧)

Re: [patch+7.8?] Fix --with-babeltrace with gcc-4.9.1

[Doug Evans]

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.

Re: [patch+7.8?] Fix --with-babeltrace with gcc-4.9.1

[Yao Qi]

On 08/13/2014 11:05 AM, Doug Evans wrote:
> 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.

I add something into the commit message as below.  Patch is committed
to mainline and 7.8 branch.

-- 
Yao (齐尧)

Subject: [PATCH] Fix build/17104

This patch is to fix the build error when GDB is configured as:

  CFLAGS=-Wall ./configure --with-babeltrace; make

This patch adds one line of code in configure test to use local
variable 'pos'.

Note that we append -Werror to CFLAGS to catch the warning related to
assignment to scope.  See more in this thread
https://sourceware.org/ml/gdb-patches/2014-08/msg00045.html

2014-08-13  Yao Qi  <yao@codesourcery.com>

	PR build/17104
	* configure.ac: Use local variable 'pos'.
	* configure: Regenerated.
---
 gdb/configure    | 1 +
 gdb/configure.ac | 1 +
 2 files changed, 2 insertions(+)

diff --git a/gdb/configure b/gdb/configure
index 809326a..874922d 100755
--- a/gdb/configure
+++ b/gdb/configure
@@ -15344,6 +15344,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 70d0964..61919b4 100644
--- a/gdb/configure.ac
+++ b/gdb/configure.ac
@@ -2437,6 +2437,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"));
-- 
1.9.0