Never create allocas for indirect function arguments #27687

dotdash · 2015-08-12T05:39:20Z

The assertion that argument debuginfo always has to be assigned to
allocas only exists in rustc, not in LLVM. The reason for that assertion
might have been that we used to created bad debuginfo for closures which
did indeed lead to errors when we skipped the alloca, but this was fixed
in commit 218eccf "Fix de-deduplication for closure debuginfo".

So now we can always skip the alloca for indirect arguments, even when
generating debuginfo.

rust-highfive · 2015-08-12T05:39:24Z

r? @arielb1

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

dotdash · 2015-08-12T05:39:32Z

r? @michaelwoerister

hanna-kruppe · 2015-08-12T08:31:55Z

src/librustc_trans/trans/base.rs

        let arg_datum = if !has_tupled_arg || i < arg_tys.len() - 1 {
-            if type_of::arg_is_indirect(bcx.ccx(), arg_ty)
-                    && bcx.sess().opts.debuginfo != FullDebugInfo {
+            if indirect_arg {
                // Don't copy an indirect argument to an alloca, the caller
                // already put it in a temporary alloca and gave it up, unless
                // we emit extra-debug-info, which requires local allocas :(.


The last part of this comment is outdated now, isn't it?

The assertion that argument debuginfo always has to be assigned to allocas only exists in rustc, not in LLVM. The reason for that assertion might have been that we used to created bad debuginfo for closures which did indeed lead to errors when we skipped the alloca, but this was fixed in commit 218eccf "Fix de-deduplication for closure debuginfo". So now we can always skip the alloca for indirect arguments, even when generating debuginfo.

michaelwoerister · 2015-08-13T21:23:04Z

Interesting. I remember running into lots of problems when not having allocas and LLVM didn't really have any documentation on it's requirement in this respect. But that was quite some time and LLVM updates ago and before unboxed closures were a thing in Rust. Maybe things have changed.
I assume this PR passes make check?

dotdash · 2015-08-13T22:25:53Z

Yes, it did pass make check for me. Had to wait for the new snapshot that
has the mentioned fix because without that it would fail in check-stage1
Am 13.08.2015 23:23 schrieb "Michael Woerister" notifications@github.com:

Interesting. I remember running into lots of problems when not having
allocas and LLVM didn't really have any documentation on it's requirement
in this respect. But that was quite some time and LLVM updates ago and
before unboxed closures were a thing in Rust. Maybe things have changed.
I assume this PR passes make check?

—
Reply to this email directly or view it on GitHub
#27687 (comment).

michaelwoerister · 2015-08-19T17:54:56Z

OK, let's give it a try :)

michaelwoerister · 2015-08-19T17:55:08Z

@bors: r+

bors · 2015-08-19T17:55:09Z

📌 Commit 98258e3 has been approved by michaelwoerister

bors · 2015-08-21T03:14:51Z

⌛ Testing commit 98258e3 with merge 3c7f021...

bors · 2015-08-21T04:08:36Z

💔 Test failed - auto-mac-32-opt

dotdash · 2015-08-27T19:32:15Z

@michaelwoerister Any idea what might be wrong here? gdb has no problem with those changes, and AFAICT, this PR makes rustc behave the same as clang, so I don't really know where to look.

michaelwoerister · 2015-08-28T16:31:28Z

I'll see if I can find out something over the weekend.

michaelwoerister · 2015-08-30T13:29:17Z

OK, so I can reproduce this error locally for 32bit builds and it seems that the DWARF information does not contain the entries for some parameters. E.g. in associated-types.rs for assoc_tuple<>() there is no entry for arg:

0x00000206:         TAG_subprogram [12] *
                     AT_low_pc( 0x00001c00 )
                     AT_high_pc( 0x00001c14 )
                     AT_frame_base( ebp )
                     AT_MIPS_linkage_name( "_ZN16associated_types16assoc_tuple<i32>E" )
                     AT_name( "assoc_tuple<i32>" )
                     AT_decl_file( "/Users/mw/rust-mac/src/test/debuginfo/associated-types.rs" )
                     AT_decl_line( 125 )

0x0000021b:             TAG_template_type_parameter [14]  
                         AT_type( {0x000002a7} ( i32 ) )
                         AT_name( "T" )

0x00000224:             NULL

In other cases, it is there as expected:

0x000001a8:         TAG_subprogram [12] *
                     AT_low_pc( 0x00001bb0 )
                     AT_high_pc( 0x00001bd0 )
                     AT_frame_base( ebp )
                     AT_MIPS_linkage_name( "_ZN16associated_types14assoc_arg<i32>E" )
                     AT_name( "assoc_arg<i32>" )
                     AT_decl_file( "/Users/mw/rust-mac/src/test/debuginfo/associated-types.rs" )
                     AT_decl_line( 117 )

0x000001bd:             TAG_formal_parameter [10]  
                         AT_location( fbreg -16 )
                         AT_name( "arg" )
                         AT_decl_file( "/Users/mw/rust-mac/src/test/debuginfo/associated-types.rs" )
                         AT_decl_line( 117 )
                         AT_type( {0x000002ae} ( i64 ) )

0x000001cb:             TAG_template_type_parameter [14]  
                         AT_type( {0x000002a7} ( i32 ) )
                         AT_name( "T" )

0x000001d4:             NULL

In the 64bit version, the entry does show up:

0x0000024a:         TAG_subprogram [12] *
                     AT_low_pc( 0x0000000100000b40 )
                     AT_high_pc( 0x0000000100000b5b )
                     AT_frame_base( rbp )
                     AT_MIPS_linkage_name( "_ZN16associated_types16assoc_tuple<i32>E" )
                     AT_name( "assoc_tuple<i32>" )
                     AT_decl_file( "/Users/mw/rust-mac/src/test/debuginfo/associated-types.rs" )
                     AT_decl_line( 125 )

0x00000267:             TAG_formal_parameter [13]  
                         AT_location( 0x00000039
                            0x0000000100000b40 - 0x0000000100000b50: rdi+0
                            0x0000000100000b50 - 0x0000000100000b5b: rbp-16, deref )
                         AT_name( "arg" )
                         AT_decl_file( "/Users/mw/rust-mac/src/test/debuginfo/associated-types.rs" )
                         AT_decl_line( 125 )
                         AT_type( {0x0000031d} ( (i32, i64) ) )

0x00000276:             TAG_template_type_parameter [14]  
                         AT_type( {0x0000030f} ( i32 ) )
                         AT_name( "T" )

0x0000027f:             NULL

Is it possible that the 32bit code generator will just remove the variable altogether while the 64bit one doesn't?

dotdash · 2015-09-01T13:07:32Z

Weird, I can reproduce this on Linux i686 now, too. Not sure why it didn't happen before. And, I can also reproduce the problem with clang.

dotdash · 2015-09-02T10:56:46Z

@michaelwoerister I figured this out now. It's a limitation of FastISel. On X86_64, the pointer is in a register, but on x86 it's on the stack. And apparently handling the value on the stack is something FastISel can't do yet.

https://github.com/llvm-mirror/llvm/blob/master/lib/CodeGen/SelectionDAG/FastISel.cpp#L1173

That is triggered for x86 but not for x86_64.

I'm not sure how we want to proceed here. Drop the patch or only skip the alloca when optimizations are turned on?

bors · 2015-09-03T00:27:58Z

☔ The latest upstream changes (presumably #28138) made this pull request unmergeable. Please resolve the merge conflicts.

michaelwoerister · 2015-09-06T02:29:15Z

@dotdash Cool that you found out what the problem is in LLVM!
So your thinking is that if somebody switches on debuginfo+optimizations they can't rely on variable info being there anyway and thus we can risk losing the information during codegen, right? That would make sense to me. On the other hand, what is there a big detriment in always generating the alloca when debuginfo is turned on?
As this slightly complicates the code and leads to some unexpected effects when lowering from LLVM IR to machine code, on just some platforms, I'd be for dropping the patch -- unless you can name a good reason not to, of course :)

dotdash · 2015-09-07T08:51:43Z

@michaelwoerister Well, the information is only "guaranteed" to be lost when optimizations are turned off, and thus FastISel is turned on. With optimizations turned on, the debug info is possibly preserved.

Anyway, I agree on this slightly complicating the code. So until I find a less complicated way and the problem in LLVM gets fixed, I'll close this.

rust-highfive assigned arielb1 Aug 12, 2015

rust-highfive assigned michaelwoerister and unassigned arielb1 Aug 12, 2015

hanna-kruppe reviewed Aug 12, 2015
View reviewed changes

dotdash force-pushed the arg_dbg branch from 428dc5d to 98258e3 Compare August 12, 2015 08:59

dotdash closed this Sep 7, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Never create allocas for indirect function arguments #27687

Never create allocas for indirect function arguments #27687

dotdash commented Aug 12, 2015

rust-highfive commented Aug 12, 2015

dotdash commented Aug 12, 2015

hanna-kruppe Aug 12, 2015

dotdash Aug 12, 2015

michaelwoerister commented Aug 13, 2015

dotdash commented Aug 13, 2015

michaelwoerister commented Aug 19, 2015

michaelwoerister commented Aug 19, 2015

bors commented Aug 19, 2015

bors commented Aug 21, 2015

bors commented Aug 21, 2015

dotdash commented Aug 27, 2015

michaelwoerister commented Aug 28, 2015

michaelwoerister commented Aug 30, 2015

dotdash commented Sep 1, 2015

dotdash commented Sep 2, 2015

bors commented Sep 3, 2015

michaelwoerister commented Sep 6, 2015

dotdash commented Sep 7, 2015

Never create allocas for indirect function arguments #27687

Never create allocas for indirect function arguments #27687

Conversation

dotdash commented Aug 12, 2015

rust-highfive commented Aug 12, 2015

dotdash commented Aug 12, 2015

hanna-kruppe Aug 12, 2015

Choose a reason for hiding this comment

dotdash Aug 12, 2015

Choose a reason for hiding this comment

michaelwoerister commented Aug 13, 2015

dotdash commented Aug 13, 2015

michaelwoerister commented Aug 19, 2015

michaelwoerister commented Aug 19, 2015

bors commented Aug 19, 2015

bors commented Aug 21, 2015

bors commented Aug 21, 2015

dotdash commented Aug 27, 2015

michaelwoerister commented Aug 28, 2015

michaelwoerister commented Aug 30, 2015

dotdash commented Sep 1, 2015

dotdash commented Sep 2, 2015

bors commented Sep 3, 2015

michaelwoerister commented Sep 6, 2015

dotdash commented Sep 7, 2015