Bug 734607

Created attachment 520699 [details]
Proposed patch

This patch makes the build succeed after include when included in the spec file.

I have not had time to test it on an ARM system, some additional review is surely needed.

Thanks Niels -- I appreciate your creating the patch.

Having gone through this annoying gcc-4.6 "unused-but-set-variable" 
exercise for all of the other architectures, the patch looks good.

> Having gone through this annoying gcc-4.6 "unused-but-set-variable" 
> exercise for all of the other architectures, the patch looks good.

Except for this piece, which was missing the required increment of 
elf32_arm_section_data(sec)->erratumcount:

+@@ -6035,9 +6027,6 @@
+                 {
+                   elf32_vfp11_erratum_list *newerr
+                     = bfd_zmalloc (sizeof (elf32_vfp11_erratum_list));
+-                  int errcount;
+-
+-                  errcount = ++(elf32_arm_section_data (sec)->erratumcount);
+
+                   newerr->u.b.vfp_insn = veneer_of_insn;
+

Fixed and tested OK against a few sample ARM dumpfiles I've got
hanging around.

(In reply to comment #3)
> > Having gone through this annoying gcc-4.6 "unused-but-set-variable" 
> > exercise for all of the other architectures, the patch looks good.
> 
> Except for this piece, which was missing the required increment of 
> elf32_arm_section_data(sec)->erratumcount:

Ai, I'm pretty sure that I noticed that... sorry for posting a obviously broken patch.

> Fixed and tested OK against a few sample ARM dumpfiles I've got
> hanging around.

Many thanks!

Changelog:

  * Thu Sep 01 2011 Dave Anderson <anderson> - 5.1.7-2 
  - Fixes for gcc-4.6 -Werror compile failures for ARM architecture. 

  http://koji.fedoraproject.org/koji/buildinfo?buildID=261602

Hi Dave,

can you push this update to Fedora 15 as well? The bootstrap of the new armhf architecture needs to rebuild all the packages from source, and currently crash is one of the packages that have issues.

More info on the goals and status can be found here:
- https://fedoraproject.org/wiki/Architectures/ARM/Fedora15_HardFP_Bootstrap

An update would be much appreciated! Thanks,
Niels

How complete is the ARM support now? Would it be meaningful to ask for crash support to be enabled in systemtap on arm?

(In reply to comment #6)
> Hi Dave,
> 
> can you push this update to Fedora 15 as well? The bootstrap of the new armhf
> architecture needs to rebuild all the packages from source, and currently crash
> is one of the packages that have issues.
> 
> More info on the goals and status can be found here:
> - https://fedoraproject.org/wiki/Architectures/ARM/Fedora15_HardFP_Bootstrap
> 
> An update would be much appreciated! Thanks,
> Niels

I've never done anything other than build in the devel tree.
Do you know what the fedpkg command sequence is for pushing
it into Fedora 15?

(In reply to comment #7)
> How complete is the ARM support now? Would it be meaningful to ask for crash
> support to be enabled in systemtap on arm?

ARM architecture-specific support in the crash utility was originally
implemented by a joint effort between Nokia and Sony Ericsson, and is
currently maintained by posts to the crash-utility mailing list: 

  https://www.redhat.com/mailman/listinfo/crash-utility

which get reviewed/accepted by these 3 sub-maintaners:

  mika.westerberg
  thomas.fange
  jan.karlsson

Although Mika is pretty much the primary maintainer.

As for "crash support to be enabled in systemtap", I don't understand
what you mean by that?

The sytemtap package have some form of crash integration. This was recently disabled for ARM as there was no crash support for ARM. If the ARM support in crash is now sufficiently complete we may want to request crash integration to be enabled on ARM in the systemtap package again, just as it is for primary architectures. Unfortunately I am not familiar with either of the two packages.


In either case, the first step is getting a working crash package on ARM.



As for how to push to a stable branch.

  fedpkg switch-branch f15
  [do changes as usual using git]
  fedpkg build
  fedpkg update  (publishes the build as an update)

repeat for f16.

To go back to rawhide

  fedpkg switch-branch master

> The sytemtap package have some form of crash integration. This was recently
> disabled for ARM as there was no crash support for ARM. If the ARM support in
> crash is now sufficiently complete we may want to request crash integration to
> be enabled on ARM in the systemtap package again, just as it is for primary
> architectures. Unfortunately I am not familiar with either of the two packages.

Taking a quick look at the RHEL6 systemtap package, they build an
"staplog.so" extension module for use with the crash utility.  That
requires that they:

(1) bring up a crash session on a live system (as opposed
    to a kernel-crash dumpfile), and
(2) then dynamically load the staplog.so extension module into
    to that live crash session, which
(3) effectively adds a couple of new crash commands, "staplog" 
    and  "staplog_cleanup" -- that do whatever they do...  

In any case, it wouldn't be a simple matter of just re-enabling
it for ARM in the systemtap.spec file, because I can see that 
they would at least need to update their extension module source
file staplog.c, which has this:

#ifdef __alpha__
#define ALPHA
#endif
#ifdef __i386__
#define X86
#endif
#ifdef __powerpc__
#define PPC
#endif
#ifdef __ia64__
#define IA64
#endif
#ifdef __s390__
#define S390
#endif
#ifdef __s390x__
#define S390X
#endif
#ifdef __powerpc64__
#define PPC64
#endif
#ifdef __x86_64__
#define X86_64
#endif

In order for the staplog.c module to even compile, they would
need to add:

#ifdef __arm__
#define ARM
#endif

I'm not sure whether there would be any other issues.

(In reply to comment #10)
> 
> As for how to push to a stable branch.
> 
>   fedpkg switch-branch f15
>   [do changes as usual using git]
>   fedpkg build
>   fedpkg update  (publishes the build as an update)
> 
> repeat for f16.
> 
> To go back to rawhide
> 
>   fedpkg switch-branch master

Can you clarify the "[do changes as usual using git]" part?
I want the f15 and f16 versions to be exactly the same as
the rawhide version.  So I'm presuming that after the 
switch-branch, I would do this:

  $ git add ppc64-unused-but-set-variable.patch
  $ git add arm-unused-but-set-variable.patch
  $ [manually update the crash.spec file]
  $ [manually update the sources file]
     note that the crash.spec file will reference the latest
     upstream package that was uploaded to the look-aside location
     when I did the two most recent rawhide builds
  $ fedpkg commit -p

And then do the fedpkg build and update steps.  That sound right?

Exactly. Or even simpler if you want them to be identical:

   git merge master

crash-5.1.7-2.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/crash-5.1.7-2.fc16

crash-5.1.7-2.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/crash-5.1.7-2.fc15

Package crash-5.1.7-2.fc16:
* should fix your issue,
* was pushed to the Fedora 16 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing crash-5.1.7-2.fc16'
as soon as you are able to.
Please go to the following url:
https://admin.fedoraproject.org/updates/crash-5.1.7-2.fc16
then log in and leave karma (feedback).

The F15 update still fails on ARM with

libtool: compile:  gcc -DHAVE_CONFIG_H -I. -I. -I. -I./../include -DBINDIR=\"/usr/local/bin\" -W -Wall -Wstrict-prototypes -Wmissing-prototypes -Werror -O2 -g -march=armv7-a -mfloat-abi=hard -mfpu=vfpv3-d16 -mthumb -MT elf32-arm.lo -MD -MP -MF .deps/elf32-arm.Tpo -c elf32-arm.c -o elf32-arm.o
elf32-arm.c: In function 'arm_map_one_stub':
elf32-arm.c:12965:25: error: variable 'info' set but not used [-Werror=unused-but-set-variable]

Created attachment 523741 [details]
Updated patch

The attached updated patch fixes two remaining unused variables

Niels, I'm curious how you were able to do a successful ARM scratch
build without this additional piece: 

+--- gdb-7.0/bfd/cpu-arm.c.orig	2011-09-18 05:34:17.794137142 -0400
+@@ -228,7 +228,6 @@
+ {
+   unsigned long namesz;
+   unsigned long descsz;
+-  unsigned long type;
+   char *        descr;
+ 
+   if (buffer_size < offsetof (arm_Note, name))
+@@ -238,7 +237,6 @@
+      host whose endian-ness is different from the target.  */
+   namesz = bfd_get_32 (abfd, buffer);
+   descsz = bfd_get_32 (abfd, buffer + offsetof (arm_Note, descsz));
+-  type   = bfd_get_32 (abfd, buffer + offsetof (arm_Note, type));
+   descr  = (char *) buffer + offsetof (arm_Note, name);
+ 
+   /* Check for buffer overflow.  */

> Niels, I'm curious how you were able to do a successful ARM scratch
> build without this additional piece: 

Henrik's patch confused me because it's not addressing just the
two additional unused items, but rather re-creates the current
arm patch.  So there's also this one:

elf32-arm.c: In function 'arm_map_one_stub':
elf32-arm.c:12965:25: error: variable 'info' set but not used
[-Werror=unused-but-set-variable]

Still don't understand why your scratch build worked?

That scratch build was on F14, while I am building on F15 if that makes any difference. Not sure if F14 have unused variables as an error condition.

If I'm not mistaken, this was a compiler feature introduced in gcc-4.6.
The latest F14 gcc version seems to be gcc-4.5.1-5.fc14.  So I'm still
confused as to how/where Niels saw the problem.

Of interesting to note: the latest upstream version, gdb-7.3.1, has
updates that address all of the "unused-but-set-variable" items, and
has the removal of the "info" variable in elf32-arm.c.  But it doesn't
remove the "type" variable from cpu-arm.c, but rather they just
puts it on a line by itself without actually "using" it.

Following their lead, the additional patch would look like this:

--- gdb-7.0/bfd/elf32-arm.c.orig
+++ gdb-7.0/bfd/elf32-arm.c
@@ -12962,7 +12962,6 @@ arm_map_one_stub (struct bfd_hash_entry 
                  void * in_arg)
 {
   struct elf32_arm_stub_hash_entry *stub_entry;
-  struct bfd_link_info *info;
   asection *stub_sec;
   bfd_vma addr;
   char *stub_name;
@@ -12977,8 +12976,6 @@ arm_map_one_stub (struct bfd_hash_entry 
   stub_entry = (struct elf32_arm_stub_hash_entry *) gen_entry;
   osi = (output_arch_syminfo *) in_arg;
 
-  info = osi->info;
-
   stub_sec = stub_entry->stub_sec;
 
   /* Ensure this stub is attached to the current section being
--- gdb-7.0/bfd/cpu-arm.c.orig
+++ gdb-7.0/bfd/cpu-arm.c
@@ -262,6 +262,7 @@ arm_check_note (bfd *abfd,
     }
 
   /* FIXME: We should probably check the type as well.  */
+  (void) type;
 
   if (description_return != NULL)
     * description_return = descr;

Anyway, I'll do this -- based upon the latest upstream version
of crash (5.1.8), I'll patch its gdb-7.0.patch with the patch
snippet above, and create a scratch src.rpm.

Then before doing anything else, I'll make that test src.rpm available
for you guys to test on a real ARM machine on both Fedora releases.
After you verify that it builds on ARM, I'll update the rawhide, fc15
and fc16 branches, and try another "fedpkg update" on fc15 and fc16.

Although -- I don't know how to stop the two current fc15 and fc16
fedpkg-updates-in-the-making.  Do you know of an "update-abort" switch
somewhere?

It's not required to abort the update, but if you really want to then you can do this from the bodhi web gui. Just open the links that were posted in this bug report by Fedora Update System, login and delete the update. Probably possible using the bodhi command line client as well, have never done that but I think "bodhi -d crash-5.1.7-2.fc16 crash-5.1.7-2.fc15"  should work.

Alternatively you can let it be, and either submit a new update which will automatically obsolete the bad one, or edit the existing update to reference new builds when ready.

OK, good, I'll leave them alone.

There won't be a problem giving you unchecked-in src.rpm to test, will there?

Please test whether crash-5.1.8-1.fc14.armtest.src.rpm will build 
on ARM with gcc-4.6:

  http://people.redhat.com/anderson/bz734607

Based upon your feedback, I will check it into Rawhide, and then merge
it into F15 and F16.

Building for F14 in the Fedora ARM Koji:

$ arm-koji build --scratch dist-f14 crash-5.1.8-1.fc14.armtest.src.rpm
Uploading srpm: crash-5.1.8-1.fc14.armtest.src.rpm
[====================================] 100% 00:04:29  23.30 MiB  88.40 KiB/sec
Created task: 167527
Task info: http://arm.koji.fedoraproject.org/koji/taskinfo?taskID=167527

Building in a F15-hardfp mock environment as well. Will post the results tomorrow.

Created attachment 524012 [details]
f15-hardfp build.log from mock

Created attachment 524013 [details]
f14 build.lof from koji

Both builds (koji F14 and mock F15) were successful.

My first comment #0 mentions that I did a F15 mock-build of crash which failed. I might have only tested my patch in F14 koji and did not test it in F15 mock.

Sorry for the confusion, many thanks for fixing this issue!

Rawhide changelog:

* Tue Sep 20 2011 Dave Anderson <anderson> - 5.1.8-1
- Update to latest upstream release
- Additional fixes for gcc-4.6 -Werror compile failures for ARM architecture.

Information for build crash-5.1.8-1.fc17:
  
http://koji.fedoraproject.org/koji/buildinfo?buildID=264624

F15 changelog:

* Tue Sep 20 2011 Dave Anderson <anderson> - 5.1.8-1
- Update to latest upstream release
- Additional fixes for gcc-4.6 -Werror compile failures for ARM architecture.

Information for build crash-5.1.8-1.fc15:

 http://koji.fedoraproject.org/koji/buildinfo?buildID=264627

F16 changelog:

* Tue Sep 20 2011 Dave Anderson <anderson> - 5.1.8-1
- Update to latest upstream release
- Additional fixes for gcc-4.6 -Werror compile failures for ARM architecture.

Information for build crash-5.1.8-1.fc16:

 http://koji.fedoraproject.org/koji/buildinfo?buildID=264832

crash-5.1.8-1.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/crash-5.1.8-1.fc16

crash-5.1.8-1.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/crash-5.1.8-1.fc15

Package crash-5.1.8-1.fc16:
* should fix your issue,
* was pushed to the Fedora 16 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing crash-5.1.8-1.fc16'
as soon as you are able to.
Please go to the following url:
https://admin.fedoraproject.org/updates/crash-5.1.8-1.fc16
then log in and leave karma (feedback).

Hi Henrik,

The following comment has been added to the crash-5.1.8-1.fc16 update:

-----------------------------------------------

bodhi - 2011-09-28 23:06:31 (karma: 0)
This update has reached 7 days in testing and can be pushed to stable now if the maintainer wishes

To reply to this comment, please visit the URL at the bottom of this mail

================================================================================
     crash-5.1.8-1.fc16
================================================================================
  Update ID: FEDORA-2011-13086
    Release: Fedora 16
     Status: testing
       Type: enhancement
      Karma: 0
       Bugs: 734607 - crash does not build on ARM
      Notes: Requested by Niels de Vos (ndevos) for ARM support.
           : Requested by Niels de Vos (ndevos) for ARM
           : architecture support
  Submitter: crash
  Submitted: 2011-09-21 17:45:03
   Comments: bodhi - 2011-09-21 17:45:12 (karma 0)
             This update has been submitted for testing by crash.

             autoqa - 2011-09-21 18:45:58 (karma 0)
             AutoQA: depcheck test PASSED on i386. Result log: http
             ://autoqa.fedoraproject.org/results/197603-autotest/10
             .5.124.163/depcheck/results/crash-5.1.8-1.fc16.html
             (results are informative only)

             autoqa - 2011-09-21 18:51:32 (karma 0)
             AutoQA: depcheck test PASSED on x86_64. Result log: ht
             tp://autoqa.fedoraproject.org/results/197602-autotest/
             10.5.124.164/depcheck/results/crash-5.1.8-1.fc16.html
             (results are informative only)

             bodhi - 2011-09-21 22:12:50 (karma 0)
             This update has been pushed to testing

             bodhi - 2011-09-28 23:06:31 (karma 0)
             This update has reached 7 days in testing and can be
             pushed to stable now if the maintainer wishes

  https://admin.fedoraproject.org/updates/crash-5.1.8-1.fc16

--------------------------------------------------

Does this just require a "go ahead and push it" comment by me,
or is there a fedpkg command that I am supposed to execute?

I usually just click on the provided link, log in and then click on "Push to stable" at the top right. This gives time to review any comments etc first.

If you know there have been no comments you need to respond to then you can also request to push the update to f16 stable branh using the bodhi command client:

  bodhi -R stable crash-5.1.8-1.fc16

Ah, OK -- I wasn't logged in when I first went to that page, and therefore
the "Mark as stable" link wasn't there.

Thanks for all your help,
  Dave

crash-5.1.8-1.fc16 has been pushed to the Fedora 16 stable repository.  If problems still persist, please make note of it in this bug report.

crash-5.1.8-1.fc15 has been pushed to the Fedora 15 stable repository.  If problems still persist, please make note of it in this bug report.