New user self-registration is disabled due to spam. For an account please email bugs-admin@lists.llvm.org with your e-mail address and full name.

Bug 43132 - wasm32: Miscompile of aggregate return containing i128
Summary: wasm32: Miscompile of aggregate return containing i128
Status: RESOLVED FIXED
Alias: None
Product: libraries
Classification: Unclassified
Component: Backend: WebAssembly (show other bugs)
Version: trunk
Hardware: PC All
: P enhancement
Assignee: Thomas Lively
URL:
Keywords:
Depends on:
Blocks: release-9.0.0
  Show dependency tree
 
Reported: 2019-08-27 16:25 PDT by Alex Crichton
Modified: 2019-09-05 03:50 PDT (History)
5 users (show)

See Also:
Fixed By Commit(s):


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Crichton 2019-08-27 16:25:50 PDT
First reported upstream at https://github.com/rust-lang/rust/issues/63918 this has been minimized to IR that looks like:




; ModuleID = 'foo.ll'
source_filename = "foo.ll"
target triple = "wasm32-unknown-unknown"

@foo = private constant i128 8000000000000000000

define i128 @get() {
start:
  %0 = tail call { i64, i128 } @decode(i8* bitcast (i128* @foo to i8*))
  %1 = extractvalue { i64, i128 } %0, 1
  ret i128 %1
}

declare hidden { i64, i128 } @decode(i8* nocapture readonly)




When compiled with `llc wut.ll -o foo.o -filetype=obj -O2` the generated output contains:



  (func (;1;) (type 0) (param i32)
    (local i32)
    global.get 0
    i32.const 32
    i32.sub
    local.tee 1
    global.set 0
    local.get 1
    i32.const 8
    i32.add
    i32.const 0
    call 0
    local.get 0
    local.get 1
    i64.load offset=8
    i64.store
    local.get 1
    i32.const 32
    i32.add
    global.set 0)


but at the end of the function there's only one `i64.store` instead of two, so I think that the upper or lower bits is missing from the store, meaning that not all of the i128 bytes were copied over.
Comment 1 Thomas Lively 2019-08-27 16:29:58 PDT
Very fishy. Will investigate.
Comment 2 Dan Gohman 2019-08-27 19:01:06 PDT
Interesting. Looks like a bug in the platform-independent return value lowering when there's a return that requires lowering to an out-parameter, and the return value comes from a first-class aggregate.

This fixes the testcase, though I need to think through how it handles other corner cases.

Index: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
===================================================================
--- lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp	(revision 370106)
+++ lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp	(working copy)
@@ -1809,7 +1809,7 @@
       // offsets to its parts don't wrap either.
       SDValue Ptr = DAG.getObjectPtrOffset(getCurSDLoc(), RetPtr, Offsets[i]);
 
-      SDValue Val = RetOp.getValue(i);
+      SDValue Val = RetOp.getValue(RetOp.getResNo() + i);
       if (MemVTs[i] != ValueVTs[i])
         Val = DAG.getPtrExtOrTrunc(Val, getCurSDLoc(), MemVTs[i]);
       Chains[i] = DAG.getStore(Chain, getCurSDLoc(), Val,
Comment 3 Dan Gohman 2019-08-29 16:01:22 PDT
Fix posted here: https://reviews.llvm.org/D66978

Sorry for sniping this one; I have some history with the multiple-return-value code, and it caught my attention when I saw Alex's reduced testcase.
Comment 4 Thomas Lively 2019-08-29 20:52:12 PDT
Fine by me! Thanks for digging into it.
Comment 5 Dan Gohman 2019-08-29 21:41:31 PDT
The fix is now landed, in r370430.
Comment 6 Nikita Popov 2019-08-31 01:30:58 PDT
Is it still possible to include this fix in the LLVM 9 release?
Comment 7 Hans Wennborg 2019-09-02 00:58:50 PDT
(In reply to Nikita Popov from comment #6)
> Is it still possible to include this fix in the LLVM 9 release?

I wasn't aware of this bug until now. We might be able to pick up the fix if we have to do another release candidate, otherwise it will have to wait until 9.0.1.

In any case, thanks for putting this on my radar.
Comment 8 Hans Wennborg 2019-09-05 03:50:58 PDT
(In reply to Hans Wennborg from comment #7)
> (In reply to Nikita Popov from comment #6)
> > Is it still possible to include this fix in the LLVM 9 release?
> 
> I wasn't aware of this bug until now. We might be able to pick up the fix if
> we have to do another release candidate, otherwise it will have to wait
> until 9.0.1.
> 
> In any case, thanks for putting this on my radar.

There will be an rc4, so I've merged this to release_90 in r371053.