www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - What is going on in this code?

reply Carlin <carlin.stpierre gmail.com> writes:
I have encountered a weird error that only seems to occur when 
building in debug mode. In release everything works fine. I 
wanted to post here before raising a new issue in case I'm just 
horribly blind at what is happening here.

---
import std.stdio;
import std.bitmanip;

ubyte[] serialize(uint t)
{
	ubyte[] bytes = nativeToBigEndian(t);
	return bytes;
}

void main()
{
	writeln(serialize(0));
}
---

$ dub --build=debug
Performing "debug" build using dmd for x86.
test ~master: target for configuration "application" is up to 
date.
To force a rebuild of up-to-date targets, run again with --force.
Running .\test.exe
[12, 254, 24, 0]

$ dub --build=release
Performing "release" build using dmd for x86.
test ~master: target for configuration "application" is up to 
date.
To force a rebuild of up-to-date targets, run again with --force.
Running .\test.exe
[0, 0, 0, 0]

Why is it returning a different range in debug but not in release?
Apr 14 2016
parent reply ag0aep6g <anonymous example.com> writes:
On 14.04.2016 12:14, Carlin wrote:

 import std.stdio;
 import std.bitmanip;

 ubyte[] serialize(uint t)
 {
      ubyte[] bytes = nativeToBigEndian(t);
      return bytes;
 }

 void main()
 {
      writeln(serialize(0));
 }


Your code is wrong. It's really easy to get wrong, though. Too easy 
probably.

nativeToBigEndian returns a fixed-size array. That's a value type. By 
assigning it to a ubyte[], you're slicing the return value. That is, you 
make a reference to temporary data. The reference becomes invalid as 
soon the assignment is over, so you're returning a slice to garbage memory.

To make a ubyte[] from the result of nativeToBigEndian, you have to dup it:
----
ubyte[] serialize(uint t)
{
     return nativeToBigEndian(t).dup;
}
----
Apr 14 2016
next sibling parent reply Carlin <carlin.stpierre gmail.com> writes:
On Thursday, 14 April 2016 at 10:29:43 UTC, ag0aep6g wrote:

 On 14.04.2016 12:14, Carlin wrote:

 import std.stdio;
 import std.bitmanip;

 ubyte[] serialize(uint t)
 {
      ubyte[] bytes = nativeToBigEndian(t);
      return bytes;
 }

 void main()
 {
      writeln(serialize(0));
 }


 Your code is wrong. It's really easy to get wrong, though. Too 
 easy probably.

 nativeToBigEndian returns a fixed-size array. That's a value 
 type. By assigning it to a ubyte[], you're slicing the return 
 value. That is, you make a reference to temporary data. The 
 reference becomes invalid as soon the assignment is over, so 
 you're returning a slice to garbage memory.

 To make a ubyte[] from the result of nativeToBigEndian, you 
 have to dup it:
 ----
 ubyte[] serialize(uint t)
 {
     return nativeToBigEndian(t).dup;
 }
 ----


Thanks, that works perfectly! I couldn't figure it out and I knew 
I had to be missing something really basic. I still don't 
understand though why it works in release but not in debug.
Apr 14 2016
parent reply Daniel Kozak <kozzi11 gmail.com> writes:
On Thursday, 14 April 2016 at 13:08:56 UTC, Carlin wrote:

 On Thursday, 14 April 2016 at 10:29:43 UTC, ag0aep6g wrote:

 [...]


 Thanks, that works perfectly! I couldn't figure it out and I 
 knew I had to be missing something really basic. I still don't 
 understand though why it works in release but not in debug.


because with release build dub will enable inlining. So I guess 
there is a stack frame elimination
Apr 14 2016
parent Daniel Kozak <kozzi11 gmail.com> writes:
On Thursday, 14 April 2016 at 13:30:42 UTC, Daniel Kozak wrote:

 On Thursday, 14 April 2016 at 13:08:56 UTC, Carlin wrote:

 On Thursday, 14 April 2016 at 10:29:43 UTC, ag0aep6g wrote:

 [...]


 Thanks, that works perfectly! I couldn't figure it out and I 
 knew I had to be missing something really basic. I still don't 
 understand though why it works in release but not in debug.


 because with release build dub will enable inlining. So I guess 
 there is a stack frame elimination


So if you add this:
pragma(inline, false)

before ubyte[] serialize(uint t)

It should not work even with release build type
Apr 14 2016
prev sibling parent reply Steven Schveighoffer <schveiguy yahoo.com> writes:
On 4/14/16 6:29 AM, ag0aep6g wrote:

 On 14.04.2016 12:14, Carlin wrote:

 import std.stdio;
 import std.bitmanip;

 ubyte[] serialize(uint t)
 {
      ubyte[] bytes = nativeToBigEndian(t);
      return bytes;
 }

 void main()
 {
      writeln(serialize(0));
 }


 Your code is wrong. It's really easy to get wrong, though. Too easy
 probably.

 nativeToBigEndian returns a fixed-size array. That's a value type. By
 assigning it to a ubyte[], you're slicing the return value. That is, you
 make a reference to temporary data. The reference becomes invalid as
 soon the assignment is over, so you're returning a slice to garbage memory.

 To make a ubyte[] from the result of nativeToBigEndian, you have to dup it:
 ----
 ubyte[] serialize(uint t)
 {
      return nativeToBigEndian(t).dup;
 }
 ----


That is awful.

I propose we make slicing such a return value implicitly an error. I 
can't think of a valid reason for allowing it.

I'm not the only one: https://issues.dlang.org/show_bug.cgi?id=12625

-Steve
Apr 14 2016
next sibling parent Meta <jared771 gmail.com> writes:
On Thursday, 14 April 2016 at 13:31:25 UTC, Steven Schveighoffer 
wrote:

 That is awful.

 I propose we make slicing such a return value implicitly an 
 error. I can't think of a valid reason for allowing it.

 I'm not the only one: 
 https://issues.dlang.org/show_bug.cgi?id=12625

 -Steve


Absolutely agreed. Why haven't we done it already?
Apr 14 2016
prev sibling next sibling parent Jonathan M Davis via Digitalmars-d <digitalmars-d puremagic.com> writes:
On Thursday, April 14, 2016 09:31:25 Steven Schveighoffer via Digitalmars-d 
wrote:

 That is awful.

 I propose we make slicing such a return value implicitly an error. I
 can't think of a valid reason for allowing it.

 I'm not the only one: https://issues.dlang.org/show_bug.cgi?id=12625


Totally agree, though I wish that we could take it one step further and get
rid of the implicit slicing of static arrays. It's an unsafe feature, and
once https://issues.dlang.org/show_bug.cgi?id=8838 has been fixed, and
slicing a static array is flagged as  system, allowing the implicit slicing
makes it really easy to miss that it's happening, and if multiple  system
operations are happening in a function, the slicing of the static array
could easily be missed by a programmer verifying the safety of the code in
order to mark it with  trusted, whereas if the slice were explicit, it would
be obvious. Unfortunately, I'm willing to bet that there's no way that
Walter would give his okay on making the implicit slicing illegal because of
how much code would break as a result.

- Jonathan M Davis
Apr 15 2016
prev sibling parent Jonathan M Davis via Digitalmars-d <digitalmars-d puremagic.com> writes:
On Friday, April 15, 2016 21:16:44 Jonathan M Davis via Digitalmars-d wrote:

 On Thursday, April 14, 2016 09:31:25 Steven Schveighoffer via Digitalmars-d

 wrote:

 That is awful.

 I propose we make slicing such a return value implicitly an error. I
 can't think of a valid reason for allowing it.

 I'm not the only one: https://issues.dlang.org/show_bug.cgi?id=12625


 Totally agree, though I wish that we could take it one step further and get
 rid of the implicit slicing of static arrays. It's an unsafe feature, and
 once https://issues.dlang.org/show_bug.cgi?id=8838 has been fixed, and
 slicing a static array is flagged as  system, allowing the implicit slicing
 makes it really easy to miss that it's happening, and if multiple  system
 operations are happening in a function, the slicing of the static array
 could easily be missed by a programmer verifying the safety of the code in
 order to mark it with  trusted, whereas if the slice were explicit, it would
 be obvious. Unfortunately, I'm willing to bet that there's no way that
 Walter would give his okay on making the implicit slicing illegal because
 of how much code would break as a result.


Well, for whatever it's worth, I created an enhancement request. Maybe we'll
get lucky and be able to talk Walter into it:

https://issues.dlang.org/show_bug.cgi?id=15932

8838 and 12625 still need to be fixed regardless, but if we got rid of the
implicit slicing of static arrays, then you'd at least always be able to see
what's going on and more easily catch slicing-related bugs.

- Jonathan M Davis
Apr 15 2016