rustdoc crashing on linux-sparc64 due to unaligned access #56927

Activity

1. glaubitz commented on Dec 17, 2018

   

   glaubitz

   on Dec 17, 2018

   Last edited by glaubitz

   ContributorAuthor

   More actions

   Looking at the build log (https://buildd.debian.org/status/fetch.php?pkg=rustc&arch=sparc64&ver=1.31.0%2Bdfsg1-2&stamp=1545047706&raw=0), it looks like it's affecting rustdoc only. And it looks like an issue in the crate https://github.com/raphlinus/pulldown-cmark.
2. nagisa commented on Dec 18, 2018

   

   nagisa

   on Dec 18, 2018

   Member

   More actions

   Would it be possible to disassemble the function in question? Does the failure occur with the official builds? If so, bisecting with cargo-bisect-rustc might help to quickly narrow down the cause.

   I would do it myself, but I’ve been unsuccessful in setting up sparc VMs (installed debian fails to boot, and I haven’t gotten around to dealing with it yet).

   ---

   pulldown-cmark itself has no unsafe code so the bug is definitely in either:

   1. The compiler;
   2. Libraries;
   3. Dependencies (but there aren’t many of them…)
3. nagisa

   added

   regression-from-stable-to-stablePerformance or correctness regression from one stable version to another.Performance or correctness regression from one stable version to another.

   I-nominated

   T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.Relevant to the compiler team, which will review and decide on the PR/issue.

   on Dec 18, 2018
4. glaubitz commented on Dec 18, 2018

   

   glaubitz

   on Dec 18, 2018

   Last edited by glaubitz

   ContributorAuthor

   More actions

   I am currently bisecting this, I'm almost there :).

   As for SPARC machines, we have set up a SPARC T5 VM in the gcc compile farm which every developer can use who applies for a free account at the gcc compile farm, see: https://gcc.gnu.org/wiki/CompileFarm There are both SPARC machines running Linux as well as Solaris available.
5. glaubitz commented on Dec 19, 2018

   

   glaubitz

   on Dec 19, 2018

   ContributorAuthor

   More actions

   Found the commit which introduced the bug:

   06b8c3ee5b52dc849259655578afca70cb32dda3 is the first bad commit
   commit 06b8c3ee5b52dc849259655578afca70cb32dda3
   Author: Colin Pronovost <colin.pron@live.com>
   Date:   Mon Sep 24 23:09:44 2018 -0400
   
       Rely only on base alignment and offset for computing field alignment
       
       Fix #54028
   
   :040000 040000 b5af5c83972da8e9ff9f10e923608400e82f2964 439ea89453a897b6c984eb58b7cd43836b299e3e M      src
   
6. nagisa commented on Dec 19, 2018

   

   nagisa

   on Dec 19, 2018

   Member

   More actions

   This smellls similar to #56267, although the fixes for that particular problem have been backported and are available in stable and the lint pass does not complain about pulldown_cmark in any way...
7. pnkfelix commented on Dec 20, 2018

   

   pnkfelix

   on Dec 20, 2018

   Contributor

   More actions

   discussed at T-compiler meeting. P-high, since this may be symptom of broader UB beyond the case outlined here.
8. pnkfelix

   added

   P-highHigh priorityHigh priority

   and removed

   I-nominated

   on Dec 20, 2018
9. pnkfelix

   assigned

   nagisa

   on Dec 20, 2018
10. nagisa commented on Dec 20, 2018

    

    nagisa

    on Dec 20, 2018

    Member

    More actions

    cc @eddyb do you have any idea in what way could the bisected PR have broken things related to alignment?

    The relevant PR here is #54547
11. infinity0 commented on Dec 20, 2018

    

    infinity0

    on Dec 20, 2018

    Contributor

    More actions

    Since you guys don't run tests on sparc64, is there a way to write a test for this that crashes on x86-64? I originally told @glaubitz to ignore the sparc64 failures but it looks like it was good that he chased it down!
12. nagisa commented on Dec 20, 2018

    

    nagisa

    on Dec 20, 2018

    Last edited by nagisa

    Member

    More actions

    Since you guys don't run tests on sparc64, is there a way to write a test for this that crashes on x86-64?

    We have codegen tests which do not depend on test being run (checks the generated LLVM-IR/Assembly/etc), and so can be run on any host.
13. jrtc27 commented on Dec 21, 2018

    

    jrtc27

    on Dec 21, 2018

    Last edited by jrtc27

    More actions

    Ok, here's a reduced test case that shows the incorrect output produced by rustc:

    pub struct RawParser {
        // Unused but necessary; only reproduces when active_tab isn't the
        // first field.
        off: usize,
        active_tab: [u8; 256],
    }
    
    impl RawParser {
        pub fn init_active(&mut self) {
            for &c in b"\x00\t\n\r_\\&*[!`<" {
                self.active_tab[c as usize] = 1;
            }
        }
    }

    This then generates the following IR (you need -O to unroll the loop):

    ; lib::RawParser:: init_active
    ; Function Attrs: nounwind uwtable
    define void @_ZN3lib9RawParser11init_active17hda1bc094c972134fE(%RawParser* nocapture dereferenceable(264) %self) unnamed_addr #0 {
    start:
      %0 = getelementptr inbounds %RawParser, %RawParser* %self, i64 0, i32 3, i64 0
      store i8 1, i8* %0, align 8
      %1 = getelementptr inbounds %RawParser, %RawParser* %self, i64 0, i32 3, i64 9
      store i8 1, i8* %1, align 8
      %2 = getelementptr inbounds %RawParser, %RawParser* %self, i64 0, i32 3, i64 10
      store i8 1, i8* %2, align 8
      %3 = getelementptr inbounds %RawParser, %RawParser* %self, i64 0, i32 3, i64 13
      store i8 1, i8* %3, align 8
      %4 = getelementptr inbounds %RawParser, %RawParser* %self, i64 0, i32 3, i64 95
      store i8 1, i8* %4, align 8
      %5 = getelementptr inbounds %RawParser, %RawParser* %self, i64 0, i32 3, i64 92
      store i8 1, i8* %5, align 8
      %6 = getelementptr inbounds %RawParser, %RawParser* %self, i64 0, i32 3, i64 38
      store i8 1, i8* %6, align 8
      %7 = getelementptr inbounds %RawParser, %RawParser* %self, i64 0, i32 3, i64 42
      store i8 1, i8* %7, align 8
      %8 = getelementptr inbounds %RawParser, %RawParser* %self, i64 0, i32 3, i64 91
      store i8 1, i8* %8, align 8
      %9 = getelementptr inbounds %RawParser, %RawParser* %self, i64 0, i32 3, i64 33
      store i8 1, i8* %9, align 8
      %10 = getelementptr inbounds %RawParser, %RawParser* %self, i64 0, i32 3, i64 96
      store i8 1, i8* %10, align 8
      %11 = getelementptr inbounds %RawParser, %RawParser* %self, i64 0, i32 3, i64 60
      store i8 1, i8* %11, align 8
      ret void
    }

    These elements are obviously not all 8-byte aligned, but the result is that LLVM combines adjacent stores, specifically 9/10, 91/92 and 95/96, into store i16 257, i16* ... (well, actually it seems to happen at the first instruction selection step). We're unlucky and it turns out none of these are 2-byte aligned as each of them has an odd first half of the pair.

    I believe the problem is in project_index, which does:

        pub fn project_index<Bx: BuilderMethods<'a, 'tcx, Value = V>>(
            &self,
            bx: &mut Bx,
            llindex: V
        ) -> Self {
            PlaceRef {
                llval: bx.inbounds_gep(self.llval, &[bx.cx().const_usize(0), llindex]),
                llextra: None,
                layout: self.layout.field(bx.cx(), 0),
                align: self.align
            }
        }

    Specifically, the array itself is aligned to 8 bytes and so the mentioned PR is correct to not force the array to be assumed aligned only to 1 byte (as was happening before with the extra mins), but it has exposed this flaw in project_index. Specifically, for constant/immediate llindex, it can do self.align.restrict_for_offset(index * element_size), but in general if llindex is an arbitrary non-constant expression it needs to be self.align.restrict_for_offset(element_size), i.e. assuming the worst-case of index == 1.

    Finally, here's a more minimal test case that doesn't try to look like the code from pulldown-cmark:

    pub struct S {
        pad: usize,
        arr: [u8; 3],
    }
    
    impl S {
        pub fn f(&mut self) {
            self.arr[1] = 0;
            self.arr[2] = 0;
        }
    }

    This gives (still with -O, otherwise LLVM won't merge the loads):

    ; min::S::f
    ; Function Attrs: norecurse nounwind uwtable
    define void @_ZN3min1S1f17h1ac21da989830415E(%S* nocapture dereferenceable(16) %self) unnamed_addr #0 {
    start:
      %0 = getelementptr inbounds %S, %S* %self, i64 0, i32 3, i64 1
      store i8 0, i8* %0, align 8
      %1 = getelementptr inbounds %S, %S* %self, i64 0, i32 3, i64 2
      store i8 0, i8* %1, align 8
      ret void
    }

    ❤️React with ❤️2Reacted by Hanna Kruppe and Simonas Kazlauskas
14. nikic commented on Dec 21, 2018

    

    nikic

    on Dec 21, 2018

    Contributor

    More actions

    Ugh, we already fixed two very similar issues, and this is the third. Reviewing other code that generates GEPs, we also have the same issue with Repeat handling:

    #[repr(align(64))]
    pub struct Foo([i32; 16]);
    
    pub fn test(x: &mut Foo) {
        x.0 = [17; 16];
    }
    

    Generates:

    define void @_ZN6test184test17hdf9fe1a59f5e8f01E(%Foo* dereferenceable(64) %x) unnamed_addr #0 {
    start:
      %0 = bitcast %Foo* %x to [16 x i32]*
      %1 = getelementptr inbounds [16 x i32], [16 x i32]* %0, i64 0, i64 0
      %2 = getelementptr inbounds [16 x i32], [16 x i32]* %0, i64 0, i64 16
      br label %repeat_loop_header
    
    repeat_loop_header:                               ; preds = %repeat_loop_body, %start
      %3 = phi i32* [ %1, %start ], [ %5, %repeat_loop_body ]
      %4 = icmp ne i32* %3, %2
      br i1 %4, label %repeat_loop_body, label %repeat_loop_next
    
    repeat_loop_body:                                 ; preds = %repeat_loop_header
      store i32 17, i32* %3, align 64
      %5 = getelementptr inbounds i32, i32* %3, i64 1
      br label %repeat_loop_header
    
    repeat_loop_next:                                 ; preds = %repeat_loop_header
      ret void
    }
    

    Notably, that store is not going to be 64 byte aligned for all array indexes...
15. jrtc27 commented on Dec 21, 2018

    

    jrtc27

    on Dec 21, 2018

    More actions

    I think something like this is what you want:

        pub fn project_index<Bx: BuilderMethods<'a, 'tcx, Value = V>>(
            &self,
            bx: &mut Bx,
            llindex: V
        ) -> Self {
            let index = if bx.cx().is_const_integral(llindex) { bx.cx().const_to_uint(llindex) } else { 1 };
            let layout = self.layout.field(bx.cx(), 0);
            PlaceRef {
                llval: bx.inbounds_gep(self.llval, &[bx.cx().const_usize(0), llindex]),
                llextra: None,
                layout: layout,
                align: self.align.restrict_for_offset(index * layout.size)
            }
        }
16. nikic commented on Dec 21, 2018

    

    nikic

    on Dec 21, 2018

    Contributor

    More actions

    I've submitted #57053 to fix both issues.
17. Centril

    added 5 commits that reference this issue on Dec 22, 2018

    Rollup merge of rust-lang#57053 - nikic:fix-gep-align, r=nagisa

    ...

    This issue will close once commit aefca41 is merged into the 'master' branch.

    aefca41

    Rollup merge of rust-lang#57053 - nikic:fix-gep-align, r=nagisa

    ...

    This issue will close once commit 35fa309 is merged into the 'master' branch.

    35fa309

    Rollup merge of rust-lang#57053 - nikic:fix-gep-align, r=nagisa

    ...

    This issue will close once commit 841af07 is merged into the 'master' branch.

    841af07

    Rollup merge of rust-lang#57053 - nikic:fix-gep-align, r=nagisa

    ...

    This issue will close once commit 4ebc7b7 is merged into the 'master' branch.

    4ebc7b7
18. bors

    closed this as completedin #57053on Dec 24, 2018
19. nikic commented on Dec 24, 2018

    

    nikic

    on Dec 24, 2018

    Contributor

    More actions

    Reopening to track beta backport.
20. nikic

    reopened this on Dec 24, 2018
21. nikic commented on Jan 4, 2019

    

    nikic

    on Jan 4, 2019

    Contributor

    More actions

    Beta backport landed as part of #57305.
22. nikic

    closed this as completedon Jan 4, 2019