guix: Use gcc-8 across the board #21799

pull dongcarl wants to merge 1 commits into bitcoin:master from dongcarl:2021-04-guix-gcc-memcmp changing 2 files +3 −3
  1. dongcarl commented at 7:26 pm on April 28, 2021: member

    Only non-base commit is the last commit: https://github.com/bitcoin/bitcoin/commit/b5abb07d0d8051f337df2e7981d42a60e0140ee3

    Right now, here’s what we use in Gitian:

    In Guix right now we use gcc-9 across the board.

    I think it makes more sense to use gcc-8 across the board, as it doesn’t suffer from the memcmp bug, and is what debian buster (stable) does, meaning it will be well tested (g++-mingw-w64, g++-aarch64-linux-gnu).

    We can accomplish this somewhat easily using Guix as we have tighter control over the toolchain (see: https://github.com/bitcoin/bitcoin/commit/b5abb07d0d8051f337df2e7981d42a60e0140ee3).

    Let me know your thoughts!

  2. hebasto commented at 7:57 pm on April 28, 2021: member
    Concept ACK on keeping the same compiler while migrating from gitian to guix.
  3. dongcarl commented at 8:04 pm on April 28, 2021: member

    Concept ACK on keeping the same compiler while migrating from gitian to guix.

    Note that we won’t be keeping the same compiler for mingw-w64 (but I think that’s okay) :-)

  4. DrahtBot added the label Scripts and tools on Apr 28, 2021
  5. DrahtBot commented at 5:29 am on April 29, 2021: member

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    No conflicts as of last run.

  6. MarcoFalke commented at 7:46 am on April 29, 2021: member

    The switch from gitian->guix will be a major bump, so I think we don’t need to use the exact same versions for all packages. However, looking at Debian (and Ubuntu) to see which packages are well-propagated among users (and thus tested) makes sense. I presume, once Debian/Ubuntu upgrade from gcc-10.2 to gcc-10.3, we could use that.

    Concept ACK to prefer 8 over 9 in the meantime.

  7. sipa commented at 7:48 am on April 29, 2021: member
    I think using GCC 8 is decent choice.
  8. guix: Consistently use gcc-8 for $HOST c90f6e5109
  9. dongcarl force-pushed on May 3, 2021
  10. dongcarl marked this as ready for review on May 3, 2021
  11. dongcarl commented at 6:44 pm on May 3, 2021: member

    Pushed b5abb07d0d8051f337df2e7981d42a60e0140ee3 -> c90f6e51094a1ba4fb2aab35b78f23b6fda645d0

    • Updated to be based on master instead of on #21462
    • Updated commit description
  12. MarcoFalke added the label Needs Guix build on May 3, 2021
  13. MarcoFalke commented at 6:46 pm on May 3, 2021: member
    Approach ACK c90f6e51094a1ba4fb2aab35b78f23b6fda645d0, haven’t reviewed
  14. in contrib/guix/libexec/build.sh:258 in c90f6e5109
    254@@ -255,7 +255,7 @@ case "$HOST" in
    255 esac
    256 
    257 case "$HOST" in
    258-    powerpc64-linux-*) HOST_LDFLAGS="${HOST_LDFLAGS} -Wl,-z,noexecstack" ;;
    259+    powerpc64-linux-*|riscv64-linux-*) HOST_LDFLAGS="${HOST_LDFLAGS} -Wl,-z,noexecstack" ;;
    


    hebasto commented at 7:52 pm on May 3, 2021:
    Why this change?

    dongcarl commented at 8:03 pm on May 3, 2021:
    It was required to pass the NX check for riscv64, I’m guessing the default execstack behavior changed between gcc 8 and 9

    dongcarl commented at 8:35 pm on May 3, 2021:

    Found it!

     0commit 12af29ab10fb5677cf947bcd7530c5f57c97522e
     1Author: Jim Wilson <jimw@sifive.com>
     2Date:   Sat Jul 14 20:05:39 2018 +0000
     3
     4    RISC-V: Fix nested function trampolines.
     5    
     6            gcc/
     7            * config/riscv/linux.h (TARGET_ASM_FILE_END): New.
     8    
     9    From-SVN: r262660
    10
    11diff --git a/gcc/ChangeLog b/gcc/ChangeLog
    12index 776a3c4434d..9179987e98c 100644
    13--- a/gcc/ChangeLog
    14+++ b/gcc/ChangeLog
    15@@ -1,3 +1,7 @@
    16+2018-07-14  Jim Wilson  <jimw@sifive.com>
    17+
    18+	* config/riscv/linux.h (TARGET_ASM_FILE_END): New.
    19+
    20 2018-07-14  Paul Koning  <ni1d@arrl.net>
    21 
    22 	* config/pdp11/pdp11.c (pdp11_rtx_costs): Bugfixes.
    23diff --git a/gcc/config/riscv/linux.h b/gcc/config/riscv/linux.h
    24index 85561846dad..e208c95fe13 100644
    25--- a/gcc/config/riscv/linux.h
    26+++ b/gcc/config/riscv/linux.h
    27@@ -66,3 +66,5 @@ along with GCC; see the file COPYING3.  If not see
    28       %{rdynamic:-export-dynamic} \
    29       -dynamic-linker " GNU_USER_DYNAMIC_LINKER "} \
    30     %{static:-static}}"
    31+
    32+#define TARGET_ASM_FILE_END file_end_indicate_exec_stack
    
     0$ git tag --contains 12af29ab10fb5677cf947bcd7530c5f57c97522e
     1basepoints/gcc-10
     2basepoints/gcc-11
     3basepoints/gcc-12
     4misc/cutover-git
     5misc/first-auto-changelog
     6misc/first-auto-changelog-10
     7misc/first-auto-changelog-9
     8releases/gcc-10.1.0
     9releases/gcc-10.2.0
    10releases/gcc-10.3.0
    11releases/gcc-11.1.0
    12releases/gcc-9.1.0
    13releases/gcc-9.2.0
    14releases/gcc-9.3.0
    

    laanwj commented at 2:36 pm on May 4, 2021:
    Correct, this used to be needed for RISC-V as well. We did have it in the gitian build. However, for the gitian build it was removed in 2ecaf214331b506ebfac4f4922241744357d652b. (I don’t understand why it’s no longer needed on Focal’s g++8, maybe Ubuntu is patching it somehow?)

    fanquake commented at 7:13 am on May 5, 2021:

    I’ve double-checked that when building RISC-V binaries using g++-8-riscv64-linux-gnu on Focal (20.04), the stack isn’t being marked as executable. I did the following inside a new Ubuntu Focal Docker container:

     0# binutils for readelf
     1apt update && apt install g++-8-riscv64-linux-gnu binutils-riscv64-linux-gnu binutils
     2
     3# riscv64-linux-gnu-gcc-8 --version
     4riscv64-linux-gnu-gcc-8 (Ubuntu 8.4.0-3ubuntu1) 8.4.0
     5
     6# compile minimal binary
     7echo "int main() { return 0; }" > test.c
     8riscv64-linux-gnu-gcc-8 test.c -o test
     9
    10# check stack
    11readelf -lW test|grep GNU_STACK
    12  GNU_STACK      0x000000 0x0000000000000000 0x0000000000000000 0x000000 0x000000 RW  0x10
    13
    14
    15# recompile, tell linker to mark stack as executable
    16riscv64-linux-gnu-gcc-8 test.c -o test -Wl,-z,execstack
    17
    18# recheck stack
    19readelf -lW test|grep GNU_STACK
    20  GNU_STACK      0x000000 0x0000000000000000 0x0000000000000000 0x000000 0x000000 RWE 0x10
    

    If you perform the same inside a Ubuntu Bionic (18.04) container:

     0# binutils for readelf
     1apt update && apt install g++-8-riscv64-linux-gnu binutils-riscv64-linux-gnu binutils
     2
     3# riscv64-linux-gnu-gcc-8 --version
     4riscv64-linux-gnu-gcc-8 (Ubuntu 8.4.0-1ubuntu1~18.04) 8.4.0
     5
     6# compile minimal binary
     7echo "int main() { return 0; }" > test.c
     8riscv64-linux-gnu-gcc-8 test.c -o test
     9
    10# check stack
    11readelf -lW test|grep GNU_STACK
    12< no output >
    13
    14# recompile, tell linker to not mark stack as executable
    15riscv64-linux-gnu-gcc-8 test.c -o test -Wl,-z,noexecstack
    16
    17# recheck stack
    18readelf -lW test|grep GNU_STACK
    19  GNU_STACK      0x000000 0x0000000000000000 0x0000000000000000 0x000000 0x000000 RW  0x10
    

    So it’s less that the stack is being marked as executable when building on Bionic, and more that the GNU_STACK section is just missing entirely, unless you explicitly tell the linker you want either a executable or non-executable stack. This makes sense when combined with the change linked by @dongcarl above (more context here), as the GNU_STACK section only started being added by default in GCC 9+. However it doesn’t explain why the section is being added when using GCC 8 on Focal (and no additional linker flags). My assumption was that the change had been backported / applied by Debian/Ubuntu, however I can’t seem to find evidence of that so far.


    laanwj commented at 8:29 am on May 5, 2021:

    Thanks for checking!

    So it’s less that the stack is being marked as executable when building on Bionic, and more that the GNU_STACK section is just missing entirely

    Right—not having a GNU_STACK section is effectively the same as having a GNU_STACK section with RWE, so, executable stack. Our security check takes this into account.

  15. hebasto approved
  16. hebasto commented at 9:20 pm on May 3, 2021: member
    ACK c90f6e51094a1ba4fb2aab35b78f23b6fda645d0, I have reviewed the code and it looks OK, I agree it can be merged.
  17. laanwj commented at 8:42 am on May 5, 2021: member
    No strong opinion on what gcc to use, but I think the argument is fair that going with what we know is the safest choice. The change looks consistent, in any case. Code review ACK c90f6e51094a1ba4fb2aab35b78f23b6fda645d0
  18. laanwj merged this on May 5, 2021
  19. laanwj closed this on May 5, 2021

  20. dongcarl deleted the branch on May 5, 2021
  21. MarcoFalke removed the label Needs Guix build on May 5, 2021
  22. sidhujag referenced this in commit 0b8ae457a2 on May 5, 2021
  23. fanquake added this to the "Done" column in a project

  24. gwillen referenced this in commit 2e922f24f4 on Jun 1, 2022
  25. fanquake referenced this in commit 2dcf3e153f on Jun 8, 2022
  26. laanwj referenced this in commit 5174a139c9 on Jun 13, 2022
  27. sidhujag referenced this in commit 8951a7ae39 on Jun 13, 2022
  28. dekm referenced this in commit 5c1e35944b on Oct 27, 2022
  29. dekm referenced this in commit 7be91387dd on Nov 7, 2022
  30. dekm referenced this in commit 70b22f0f88 on Nov 12, 2022
  31. knst referenced this in commit c2f14e4409 on Feb 23, 2023
  32. knst referenced this in commit bd846f38cf on Mar 6, 2023
  33. knst referenced this in commit 53ed8ce2ca on Mar 13, 2023
  34. PastaPastaPasta referenced this in commit 67efaa8808 on Mar 26, 2023
  35. dekm referenced this in commit 563e5c6d31 on Apr 6, 2023
  36. DrahtBot locked this on Jun 12, 2023

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-06 19:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me