Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set .llvmbc and .llvmcmd sections as allocatable #77961

Merged
merged 1 commit into from
Oct 17, 2020

Conversation

glandium
Copy link
Contributor

This marks both sections as allocatable rather than excluded, which matches what
clang does with the equivalent -fembed-bitcode flag.

@rust-highfive
Copy link
Collaborator

r? @oli-obk

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 15, 2020
@glandium
Copy link
Contributor Author

Please note that I'm not entirely sure what side effects this may have on rust. I just noticed this didn't match what clang does because I was trying to extract the bytecode with objcopy -O binary -j .llvmbc, and that didn't work because of https://sourceware.org/git/?p=binutils.git;a=blob;f=bfd/binary.c;h=700c862f93d68fd5270482eb73703147a2a9d279;hb=a0a1bb07cb2c03b7d34f12e734c6f363ddb7c7b2#l279 (but worked on clang-issues object files)

@oli-obk
Copy link
Contributor

oli-obk commented Oct 15, 2020

Uh... I have no idea what this all is.. Do you know anyone who could review this?

@mati865
Copy link
Contributor

mati865 commented Oct 15, 2020

I think @nagisa or @nikic should be able to review it.

@nagisa
Copy link
Member

nagisa commented Oct 15, 2020

This would mean that these sections are loaded into memory when the rust built-code is executed.

Alternatively, and more critically, this would mean that contents of these sections would end up being written into the program memory on embedded devices, despite it having no functional purpose in a running program, thus wasting the already precious bytes.

I think your use-case would be served sufficiently by marking these sections as noload, like they are on windows/uefi targets above the lines you changed. It would ensure that the linker wouldn't drop these sections while linking objects/binaries together, but would still prevent the section from materializing when the binary is loaded into memory for execution or is flashed into an embedded device.

r? @nagisa

@rust-highfive rust-highfive assigned nagisa and unassigned oli-obk Oct 15, 2020
@nagisa
Copy link
Member

nagisa commented Oct 15, 2020

Huh, looks like ELF assembly does not support the noload flag like the PE one does. Instead the section type becomes a SHT_NOBITS with the ALLOC flag, maybe? Can you make sure that these sections don't get loaded/included when converted to e.g. a .bin?

@glandium
Copy link
Contributor Author

glandium commented Oct 15, 2020

With the SHT_ALLOC bit set only, none of lld, gold or ld.bfd keep the llvm sections, presumably because their default link script doesn't include them. They would be kept if they were named .text.llvm*.

@nagisa
Copy link
Member

nagisa commented Oct 16, 2020

Checking the clang produced output, it seems like this data is indeed loaded into binary memory. I'd consider it a clang bug, but I guess, this PR matches clang's behaviour.

@bors r+

@bors
Copy link
Contributor

bors commented Oct 16, 2020

📌 Commit 684d142 has been approved by nagisa

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 16, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Oct 17, 2020
Set .llvmbc and .llvmcmd sections as allocatable

This marks both sections as allocatable rather than excluded, which matches what
clang does with the equivalent `-fembed-bitcode` flag.
This was referenced Oct 17, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 17, 2020
Rollup of 6 pull requests

Successful merges:

 - rust-lang#76199 (Permit uninhabited enums to cast into ints)
 - rust-lang#77751 (liballoc: VecDeque: Add binary search functions)
 - rust-lang#77785 (Remove compiler-synthesized reexports when documenting)
 - rust-lang#77932 (BTreeMap: improve gdb introspection of BTreeMap with ZST keys or values)
 - rust-lang#77961 (Set .llvmbc and .llvmcmd sections as allocatable)
 - rust-lang#77985 (llvm: backport SystemZ fix for AGR clobbers)

Failed merges:

r? `@ghost`
@bors bors merged commit 55f9676 into rust-lang:master Oct 17, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 17, 2020
@JohnTitor
Copy link
Member

@nagisa @glandium The libc build for mips64(el)-linux-gnuabi64 is failed between nightly-2020-10-17 and nightly-2020-10-18, and I guess this is the cause.
Here's the log: https://github.com/rust-lang/libc/runs/1274815350?check_suite_focus=true

  = note: /usr/lib/gcc-cross/mips64-linux-gnuabi64/9/../../../../mips64-linux-gnuabi64/bin/ld: /checkout/target/mips64-unknown-linux-gnuabi64/debug/deps/main-ab030a3be80ad042.2ubbkpxxnbb3pjbz.rcgu.o: in function `main::fn_tmpnam':
          /checkout/target/mips64-unknown-linux-gnuabi64/debug/build/libc-test-e989c0bf73fb391f/out/main.rs:14071: warning: the use of `tmpnam' is dangerous, better use `mkstemp'
          /checkout/target/mips64-unknown-linux-gnuabi64/debug/deps/main-ab030a3be80ad042.2ubbkpxxnbb3pjbz.rcgu.o: in function `main::fn_fchmod':
          /checkout/target/mips64-unknown-linux-gnuabi64/debug/build/libc-test-e989c0bf73fb391f/out/main.rs:10654:(.text._ZN4main9fn_fchmod17hf698f27b911348cdE+0x44): relocation truncated to fit: R_MIPS_GOT_DISP against `fchmod@@GLIBC_2.0'
          /checkout/target/mips64-unknown-linux-gnuabi64/debug/deps/main-ab030a3be80ad042.2ubbkpxxnbb3pjbz.rcgu.o: in function `main::fn_sigaltstack':
          /checkout/target/mips64-unknown-linux-gnuabi64/debug/build/libc-test-e989c0bf73fb391f/out/main.rs:83470:(.text._ZN4main14fn_sigaltstack17h68df0bd78d5927feE+0x44): relocation truncated to fit: R_MIPS_GOT_DISP against `sigaltstack@@GLIBC_2.0'
          collect2: error: ld returned 1 exit status

Any thoughts on this?

@tmandry
Copy link
Member

tmandry commented Oct 23, 2020

I don't think this change was correct. According to the PR description it only matches clang's behavior when -fembed-bitcode is specified, which is not an option people typically use.

We observed a huge increase in the size of libstd.so from 1.3M to 17.5M that was bisected to this change (or one of the other changes in the rollup, but probably this one.)

@nagisa
Copy link
Member

nagisa commented Oct 23, 2020

There are a couple situations where bitcode may be embedded:

  1. -Cembed-bitcode is specified;
  2. LTO is enabled;
  3. Target needs bitcode (think iOS).

I suspect that libstd.so is built with LTO enabled, one of the ways bitcode embedding might be enabled through. I'm open to backing this out, but from what I can tell the compiler is not behaving incorrectly.

@tmandry
Copy link
Member

tmandry commented Oct 23, 2020

Sorry, I didn't expand the diff context enough to see that this was in the embed_bitcode function.

I think we're building libstd wrong in that case. If LTO is enabled it should terminate at a dylib boundary unless -Cembed-bitcode is explicitly set, IMO.

@nagisa
Copy link
Member

nagisa commented Oct 23, 2020

That could be the case, but ultimately I think it is fine to revert this first (if this PR is indeed confirmed to be the issue) and then think about how to introduce it without causing a regression.

@glandium
Copy link
Contributor Author

I agree.

@betamos
Copy link

betamos commented Oct 23, 2020

Repro:

  • Checked out master
  • cp config.toml.example config.toml
  • ./x.py build
  • readelf -WS on a libstd.so shows that .llvmbc section is present, size ~9M

I assume this is not intended, but I'll leave it to the experts.

Full output:

readelf -WS ./build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/x86_64-unknown-linux-gnu/lib/libstd-5ccd458f55feeb13.so
There are 32 section headers, starting at offset 0xde9e28:

Section Headers:
  [Nr] Name              Type            Address          Off    Size   ES Flg Lk Inf Al
  [ 0]                   NULL            0000000000000000 000000 000000 00      0   0  0
  [ 1] .note.gnu.build-id NOTE            0000000000000270 000270 000024 00   A  0   0  4
  [ 2] .gnu.hash         GNU_HASH        0000000000000298 000298 004060 00   A  3   0  8
  [ 3] .dynsym           DYNSYM          00000000000042f8 0042f8 010260 18   A  4   1  8
  [ 4] .dynstr           STRTAB          0000000000014558 014558 039a6c 00   A  0   0  1
  [ 5] .gnu.version      VERSYM          000000000004dfc4 04dfc4 001588 02   A  3   0  2
  [ 6] .gnu.version_r    VERNEED         000000000004f550 04f550 000170 00   A  4   5  8
  [ 7] .rela.dyn         RELA            000000000004f6c0 04f6c0 010da0 18   A  3   0  8
  [ 8] .rela.plt         RELA            0000000000060460 060460 000090 18  AI  3  24  8
  [ 9] .init             PROGBITS        0000000000061000 061000 000017 00  AX  0   0  4
  [10] .plt              PROGBITS        0000000000061020 061020 000070 10  AX  0   0 16
  [11] .plt.got          PROGBITS        0000000000061090 061090 000008 08  AX  0   0  8
  [12] .text             PROGBITS        00000000000610a0 0610a0 0dc485 00  AX  0   0 16
  [13] .fini             PROGBITS        000000000013d528 13d528 000009 00  AX  0   0  4
  [14] .rodata           PROGBITS        000000000013e000 13e000 025ada 00   A  0   0 16
  [15] .llvmbc           PROGBITS        0000000000163ae0 163ae0 8a5740 00   A  0   0 16
  [16] .eh_frame_hdr     PROGBITS        0000000000a09220 a09220 009ac4 00   A  0   0  4
  [17] .eh_frame         PROGBITS        0000000000a12ce8 a12ce8 02b948 00   A  0   0  8
  [18] .gcc_except_table PROGBITS        0000000000a3e630 a3e630 002ddc 00   A  0   0  4
  [19] .tbss             NOBITS          0000000000a42978 a41978 0000c0 00 WAT  0   0  8
  [20] .init_array       INIT_ARRAY      0000000000a42978 a41978 000010 08  WA  0   0  8
  [21] .fini_array       FINI_ARRAY      0000000000a42988 a41988 000008 08  WA  0   0  8
  [22] .data.rel.ro      PROGBITS        0000000000a42990 a41990 00a518 00  WA  0   0  8
  [23] .dynamic          DYNAMIC         0000000000a4cea8 a4bea8 000230 10  WA  4   0  8
  [24] .got              PROGBITS        0000000000a4d0d8 a4c0d8 000f28 08  WA  0   0  8
  [25] .data             PROGBITS        0000000000a4e000 a4d000 000088 00  WA  0   0  8
  [26] .bss              NOBITS          0000000000a4e088 a4d088 0002d8 00  WA  0   0  8
  [27] .comment          PROGBITS        0000000000000000 a4d088 00001d 01  MS  0   0  1
  [28] .rustc            PROGBITS        0000000000000000 a4d0b0 2e9536 00      0   0 16
  [29] .symtab           SYMTAB          0000000000000000 d365e8 02ac90 18     30 4548  8
  [30] .strtab           STRTAB          0000000000000000 d61278 088a93 00      0   0  1
  [31] .shstrtab         STRTAB          0000000000000000 de9d0b 00011c 00      0   0  1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants