Skip to content

Comments

pkgsi686Linux.gdb: fix formatting for 32-bit systems#173844

Merged
vcunat merged 1 commit intoNixOS:staging-nextfrom
trofi:fix-gdb-12-for-32-bit
May 21, 2022
Merged

pkgsi686Linux.gdb: fix formatting for 32-bit systems#173844
vcunat merged 1 commit intoNixOS:staging-nextfrom
trofi:fix-gdb-12-for-32-bit

Conversation

@trofi
Copy link
Contributor

@trofi trofi commented May 21, 2022 •
edited
Loading

A few rare targets don't have clean format strings on 32-bit systems:
#171216 (comment)

/build/gdb-12.1/_build/sim/../../sim/cris/sim-if.c:575:28:
  error: format '%lx' expects argument of type 'long unsigned int',
    but argument 4 has type 'bfd_size_type' {aka 'long long unsigned int'} [-Werror=format=]

Let's disable format hardening on 32-bit systems until it's fixed upstream.

While at it disabled -Werror to avoid failures on newer and exotic
toolchain versions.

Description of changes
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@trofi trofi mentioned this pull request May 21, 2022
Merged
13 tasks
@vcunat
Copy link
Member

vcunat commented May 21, 2022

I hope it at least won't crash when reaching those prints.

@ofborg ofborg bot requested review from globin, lsix and nbp May 21, 2022 07:40
@ofborg ofborg bot added 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels May 21, 2022
@trofi
Copy link
Contributor Author

trofi commented May 21, 2022

Yeah, looking at the affected format strings at https://sourceware.org/pipermail/gdb-patches/2022-May/189288.html they only attempt to print digits and don't chase any pointers. That should only print garbage values.

Mindavi
Mindavi reviewed May 21, 2022
View reviewed changes
pkgs/development/tools/misc/gdb/default.nix Outdated
Copy link
Contributor

@Mindavi Mindavi May 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯❤

@Mindavi
Copy link
Contributor

Mindavi commented May 21, 2022

The disable-werror is really nice, but won't this cause a large number of rebuilds while we're trying to cut a release?

Could also target staging with that, maybe. Not sure what's best though.

(ca-derivations when? 😜)

@trofi
Copy link
Contributor Author

trofi commented May 21, 2022

I dunno. rebuild-amount.sh says it's ~7K packages being rebuilt. Does not sound too high for staging-next.

@trofi trofi changed the base branch from staging-next to staging May 21, 2022 19:30
@vcunat
Copy link
Member

vcunat commented May 21, 2022

  • We most likely want some fix for 32-bit gdb on staging-next already – that's going to 22.05, contrary to current staging.
  • Perhaps the fix could be conditional at this very moment, as I don't like delaying the release process.
  • Why did you choose for this instead of fixing by the patch when you've written it already? (I might find some time to review the patch a bit in that case, too.)

@trofi
pkgsi686Linux.gdb: fix formatting for 32-bit systems
63d81dd 
A few rare targets don't have clean format strings on 32-bit systems:
NixOS#171216 (comment)

    /build/gdb-12.1/_build/sim/../../sim/cris/sim-if.c:575:28:
      error: format '%lx' expects argument of type 'long unsigned int',
        but argument 4 has type 'bfd_size_type' {aka 'long long unsigned int'} [-Werror=format=]

We pull in patch pending upstream inclusion.
@trofi trofi force-pushed the fix-gdb-12-for-32-bit branch from bfaf2e4 to 63d81dd Compare May 21, 2022 20:19
@trofi trofi changed the base branch from staging to staging-next May 21, 2022 20:20
@trofi trofi requested a review from vcunat May 21, 2022 20:20
@trofi
Copy link
Contributor Author

trofi commented May 21, 2022

  • We most likely want some fix for 32-bit gdb on staging-next already – that's going to 22.05, contrary to current staging.
  • Perhaps the fix could be conditional at this very moment, as I don't like delaying the release process.

Sounds good. Retargeted against staging-next.

  • Why did you choose for this instead of fixing by the patch when you've written it already? (I might find some time to review the patch a bit in that case, too.)

I wrote the patch after the formatting hack and was not sure when it gets accepted upstream.

I added the patch to nixpkgs repository and applied it conditionally.

@trofi trofi changed the title pkgsi686Linux.gdb: disable format hardening on 32-bit systems pkgsi686Linux.gdb: fix formatting for 32-bit systems May 21, 2022
@vcunat vcunat merged commit a762388 into NixOS:staging-next May 21, 2022
@trofi trofi deleted the fix-gdb-12-for-32-bit branch May 21, 2022 20:39
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 101-500 This PR causes between 101 and 500 packages to rebuild on Linux. and removed 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels May 21, 2022
@vcunat
Copy link
Member

vcunat commented May 21, 2022

OK, so looking at the gdb diff, the main thing is length check by the compiler :-) so as the build now passes on both x86 linuxes, I believe the lengths are correct now. (On those platforms at least, but I don't expect variability in these sizes among platforms of the same pointer length.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Mindavi Mindavi Mindavi approved these changes

@lsix lsix Awaiting requested review from lsix

@nbp nbp Awaiting requested review from nbp

@globin globin Awaiting requested review from globin

@vcunat vcunat Awaiting requested review from vcunat

Assignees

No one assigned

Labels

10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 101-500 This PR causes between 101 and 500 packages to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@trofi @vcunat @Mindavi