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

rustc puts zero-initalized structs in .data instead of .bss if they contain padding #41315

Closed
whitequark opened this issue Apr 15, 2017 · 10 comments · Fixed by #48908
Closed
Labels
A-codegen Area: Code generation A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-enhancement Category: An issue proposing an enhancement or a PR with one. I-slow Issue: Problems and improvements with respect to performance of generated code.

Comments

@whitequark
Copy link
Member

E.g.:

const DMA_BUFFER_SIZE: usize = 8 * 1024 * 1024;

struct DmaRecorder {
    active:   bool,
    buffer:   [u8; DMA_BUFFER_SIZE],
    data_len: usize
}

static mut DMA_RECORDER: DmaRecorder = DmaRecorder {
    active:   false,
    buffer:   [0; DMA_BUFFER_SIZE],
    data_len: 0
};
$ size -Ax libt.rlib
t.0.o   (ex libt.rlib):
section                                             size   addr
.text                                                0x0    0x0
.text._ZN4drop17h735fab7fd9067161E                   0x6    0x0
.data._ZN1t12DMA_RECORDER17h7409bdfe01c28df8E   0x800010    0x0
.note.GNU-stack                                      0x0    0x0
.eh_frame                                           0x30    0x0
Total                                           0x800046

(and this results in a 8MB file, etc).

Note that this works:

const DMA_BUFFER_SIZE: usize = 8 * 1024 * 1024;

struct DmaRecorder {
    buffer:   [u8; DMA_BUFFER_SIZE],
    data_len: usize
}

static mut DMA_RECORDER: DmaRecorder = DmaRecorder {
    buffer:   [0; DMA_BUFFER_SIZE],
    data_len: 0
};
@whitequark
Copy link
Member Author

Looking at LLVM IR:

@_ZN1t12DMA_RECORDER17h7409bdfe01c28df8E = 
    internal global { i1, [8388608 x i8], [7 x i8], i64 } 
                    { i1 false, [8388608 x i8] zeroinitializer, [7 x i8] undef, i64 0 }, align 8

It looks like that the padding undef is troublesome.

@whitequark whitequark changed the title rustc puts zero-initalized structs in .data instead of .bss if they contain bool fields rustc puts zero-initalized structs in .data instead of .bss if they contain padding Apr 15, 2017
@whitequark
Copy link
Member Author

So I'm not sure what to do here. This is clearly also an LLVM issue, but with LLVM being supported down to 3.7, fixing just LLVM doesn't seem enough.

@hanna-kruppe
Copy link
Contributor

If it's indeed an LLVM bug, fixing LLVM and cherry-picking the patch into https://github.com/rust-lang/llvm/ may be the best we can do. This is often the case with LLVM issues, even with blatant miscompiles (e.g., #40593).

@nagisa nagisa added A-codegen Area: Code generation A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. I-slow Issue: Problems and improvements with respect to performance of generated code. labels Apr 16, 2017
@nagisa
Copy link
Member

nagisa commented Apr 16, 2017

I tagged this as I-slow in a sense that it generates an executable that is suboptimal (i.e. what is the spirit of the tag).

@hanna-kruppe
Copy link
Contributor

hanna-kruppe commented Apr 23, 2017

Relevant thread on llvm-dev: http://lists.llvm.org/pipermail/llvm-dev/2017-April/112305.html (well, not a thread just yet)

@Mark-Simulacrum Mark-Simulacrum added C-enhancement Category: An issue proposing an enhancement or a PR with one. and removed C-enhancement Category: An issue proposing an enhancement or a PR with one. labels Jul 26, 2017
@jonas-schievink
Copy link
Contributor

Still reproduces on 1.20 and nightly-2017-09-15 (1.22.0-nightly (5dfc84cfa 2017-09-14)). Did the patch ever happen?

@varkor
Copy link
Member

varkor commented Jan 25, 2018

I have a patch for this here. It's been accepted, so now it just needs to be merged and then we should be able to cherry-pick it.

jyknight pushed a commit to jyknight/llvm-monorepo that referenced this issue Feb 6, 2018
Following up on the discussion from
http://lists.llvm.org/pipermail/llvm-dev/2017-April/112305.html, undef
values are now placed in the .bss as well as null values. This prevents
undef global values taking up potentially huge amounts of space in the
.data section.

The following two lines now both generate equivalent .bss data:

@vals1 = internal unnamed_addr global [20000000 x i32] zeroinitializer, align 4
@Vals2 = internal unnamed_addr global [20000000 x i32] undef, align 4 ; previously unaccounted for

This is primarily motivated by the corresponding issue in the Rust
compiler (rust-lang/rust#41315).

Differential Revision: https://reviews.llvm.org/D41705

Patch by varkor!

llvm-svn=324424
earl pushed a commit to earl/llvm-mirror that referenced this issue Feb 6, 2018
Following up on the discussion from
http://lists.llvm.org/pipermail/llvm-dev/2017-April/112305.html, undef
values are now placed in the .bss as well as null values. This prevents
undef global values taking up potentially huge amounts of space in the
.data section.

The following two lines now both generate equivalent .bss data:

@vals1 = internal unnamed_addr global [20000000 x i32] zeroinitializer, align 4
@Vals2 = internal unnamed_addr global [20000000 x i32] undef, align 4 ; previously unaccounted for

This is primarily motivated by the corresponding issue in the Rust
compiler (rust-lang/rust#41315).

Differential Revision: https://reviews.llvm.org/D41705

Patch by varkor!



git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@324424 91177308-0d34-0410-b5e6-96231b3b80d8
@varkor
Copy link
Member

varkor commented Feb 6, 2018

This has now been merged in. Should be ready for cherry-picking!

@japaric
Copy link
Member

japaric commented Mar 9, 2018

This has now been merged in. Should be ready for cherry-picking!

Yay! Thanks @varkor!

If someone wants to drive this to the finish line the backporting process looks like this:

  • Fork rust-lang/llvm. Make a new branch -- base it off the rust-llvm-release-6-0-0 branch. Cherry pick the changes (https://reviews.llvm.org/rL324424). Send a PR to rust-lang/llvm (target branch is rust-llvm-release-6-0-0).

  • Once that PR is merged, fork rust-lang/rust. Make a new branch -- base it off the master branch. Update the src/llvm submodule to point to the new HEAD of the rust-llvm-release-6-0-0 branch. Send a PR with the updated submodule to rust-lang/rust (target branch is master).

  • Cross your fingers and hope that bors likes the changes 🙂

Feel free to cc me on those PRs. I can only r+ the rust-lang/rust PR, though.

varkor pushed a commit to varkor/llvm that referenced this issue Mar 9, 2018
Following up on the discussion from
http://lists.llvm.org/pipermail/llvm-dev/2017-April/112305.html, undef
values are now placed in the .bss as well as null values. This prevents
undef global values taking up potentially huge amounts of space in the
.data section.

The following two lines now both generate equivalent .bss data:

@vals1 = internal unnamed_addr global [20000000 x i32] zeroinitializer, align 4
@Vals2 = internal unnamed_addr global [20000000 x i32] undef, align 4 ; previously unaccounted for

This is primarily motivated by the corresponding issue in the Rust
compiler (rust-lang/rust#41315).

Differential Revision: https://reviews.llvm.org/D41705

Patch by varkor!



git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@324424 91177308-0d34-0410-b5e6-96231b3b80d8
@varkor
Copy link
Member

varkor commented Mar 9, 2018

@japaric: thanks for laying out the steps! I always meant to follow up here, but never got around to it (and wasn't sure what the process was on the rust/llvm end). I've followed the first steps: hopefully we can soon get this fix in for good! :)

varkor added a commit to varkor/rust that referenced this issue Mar 10, 2018
bors added a commit that referenced this issue Mar 11, 2018
Merge LLVM fix for undefined bss globals

This fixes #41315.

r? @japaric
siretty pushed a commit to siretty/llvm-project that referenced this issue Feb 17, 2019
Following up on the discussion from
http://lists.llvm.org/pipermail/llvm-dev/2017-April/112305.html, undef
values are now placed in the .bss as well as null values. This prevents
undef global values taking up potentially huge amounts of space in the
.data section.

The following two lines now both generate equivalent .bss data:

@vals1 = internal unnamed_addr global [20000000 x i32] zeroinitializer, align 4
@Vals2 = internal unnamed_addr global [20000000 x i32] undef, align 4 ; previously unaccounted for

This is primarily motivated by the corresponding issue in the Rust
compiler (rust-lang/rust#41315).

Differential Revision: https://reviews.llvm.org/D41705

Patch by varkor!

llvm-svn: 324424
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-enhancement Category: An issue proposing an enhancement or a PR with one. I-slow Issue: Problems and improvements with respect to performance of generated code.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants