On Sun, Nov 15, 2020 at 01:43:14PM +0000, donallen via Digitalmars-d wrote:
> With the help of an excellent suggestion by Steve Schveighoffer, I finally identified this problem yesterday. It was my error, not D's.
[...]
> void bind_text(sqlite3_stmt* stmt, int index, const char[] the_text)
> {
>     if (sqlite3_bind_text(stmt, index, toStringz(the_text), -1, null)
> != sqlite_ok)
>     {
>         stderr.writeln("bind_text failed");
>         auto db = sqlite3_db_handle(stmt);
>         stderr.writeln("Error: %s", fromStringz(sqlite3_errmsg(db)));
>         exit(EXIT_FAILURE);
>     }
> }
[...]
> The culprit is the call to toStringz. The documentation for that function give some good advice that I failed to follow: "Important Note: When passing a char* to a C function, and the C function keeps it around for any reason, make sure that you keep a reference to it in your D code. Otherwise, it may become invalid during a garbage collection cycle and cause a nasty bug when the C code tries to use it."

Ouch.  I'm dismayed that I missed this when I last looked at this code (which I believe you did post here). ;-)  Passing a GC-allocated string to C code without retaining a reference somewhere on the D side is a known gotcha when interfacing with C.  In fact, I'm almost certain I ran into this very problem with toStringz myself in the past, yet I still failed to notice it when I looked at your code.

In any case, I'm glad that this means you'll be around here more. ;-)


--T

On Sunday, 15 November 2020 at 13:43:14 UTC, donallen wrote:
> With the help of an excellent suggestion by Steve Schveighoffer, I finally identified this problem yesterday. It was my error, not D's.

Glad you fixed the problem.

And it looks like you missed my post:

https://forum.dlang.org/post/hqpppumvdtkbmghavsoq@forum.dlang.org


D has been around for ~20 years, and there are 1899 packages, some of them under heavy industrial use, it's very unlikely it's a GC bug, if there is a bug, people may run into it long before you do esp you are new to D.

https://code.dlang.org/

On 11/15/20 10:05 AM, H. S. Teoh wrote:
> On Sun, Nov 15, 2020 at 01:43:14PM +0000, donallen via Digitalmars-d wrote:
>> With the help of an excellent suggestion by Steve Schveighoffer, I
>> finally identified this problem yesterday. It was my error, not D's.
> [...]
>> void bind_text(sqlite3_stmt* stmt, int index, const char[] the_text)
>> {
>>      if (sqlite3_bind_text(stmt, index, toStringz(the_text), -1, null)
>> != sqlite_ok)
>>      {
>>          stderr.writeln("bind_text failed");
>>          auto db = sqlite3_db_handle(stmt);
>>          stderr.writeln("Error: %s", fromStringz(sqlite3_errmsg(db)));
>>          exit(EXIT_FAILURE);
>>      }
>> }
> [...]
>> The culprit is the call to toStringz. The documentation for that
>> function give some good advice that I failed to follow:
>> "Important Note: When passing a char* to a C function, and the C
>> function keeps it around for any reason, make sure that you keep a
>> reference to it in your D code. Otherwise, it may become invalid
>> during a garbage collection cycle and cause a nasty bug when the C
>> code tries to use it."
> 
> Ouch.  I'm dismayed that I missed this when I last looked at this code
> (which I believe you did post here). ;-)  Passing a GC-allocated string
> to C code without retaining a reference somewhere on the D side is a
> known gotcha when interfacing with C.  In fact, I'm almost certain I ran
> into this very problem with toStringz myself in the past, yet I still
> failed to notice it when I looked at your code.

I had the same reaction! I also did not see the toStringz as a problem when I reviewed.

I will note, also, that toStringz has this quirk where in certain cases it just returns the pointer of the array, which is why I think the .idup fix worked (that was puzzling to me and I think pulled me away from it being a problem with storing a pointer in C-land only).

https://github.com/dlang/phobos/blob/e89d9d9ce072269e80da4f901ce3e953d736cb82/std/string.d#L361-L367

> In any case, I'm glad that this means you'll be around here more. ;-)

Likewise!

-Steve

Wahoo!

On 11/15/20 10:43 AM, Steven Schveighoffer wrote:

> I also did not see the toStringz as a problem
> when I reviewed.

While I have everbody's attention, :) toStringz will come up during one of my DConf presentations. I will claim that it is safe to pass to a C function for their immediate consumption (but they can't save the pointer).

So, I will claim that the following is safe and no GC collection on another thread can mess this up. (The C side is not allowed to store for later use but they are taking the pointer into a local variable on their function call stack.)

nothrow extern(C) void bar(const(char) ** name) {
  // ...
  *name = makeString(42).toStringz;
  // ...
}

My claim is based on my understanding on the following thread. (Thank you, Rikki Cattermole.)

  https://forum.dlang.org/thread/rn3ii0$jjj$1@digitalmars.com

Ali

On Sun, Nov 15, 2020 at 01:46:52PM -0800, Ali Çehreli via Digitalmars-d wrote: [...]
> While I have everbody's attention, :) toStringz will come up during one of my DConf presentations. I will claim that it is safe to pass to a C function for their immediate consumption (but they can't save the pointer).
> 
> So, I will claim that the following is safe and no GC collection on another thread can mess this up. (The C side is not allowed to store for later use but they are taking the pointer into a local variable on their function call stack.)
> 
> nothrow extern(C) void bar(const(char) ** name) {
>   // ...
>   *name = makeString(42).toStringz;
>   // ...
> }
[...]

AFAIK, as long as a reference exists on the stack, you should be safe, because the GC does scan the stack.  But if the C side stores it anywhere else past the point where the on-stack reference goes out of scope, then you're in trouble.


T

-- 
Chance favours the prepared mind. -- Louis Pasteur

On 11/16/20 2:26 PM, H. S. Teoh wrote:
> On Sun, Nov 15, 2020 at 01:46:52PM -0800, Ali Çehreli via Digitalmars-d wrote:
> [...]
>> While I have everbody's attention, :) toStringz will come up during
>> one of my DConf presentations. I will claim that it is safe to pass to
>> a C function for their immediate consumption (but they can't save the
>> pointer).
>>
>> So, I will claim that the following is safe and no GC collection on
>> another thread can mess this up. (The C side is not allowed to store
>> for later use but they are taking the pointer into a local variable on
>> their function call stack.)
>>
>> nothrow extern(C) void bar(const(char) ** name) {
>>    // ...
>>    *name = makeString(42).toStringz;
>>    // ...
>> }
> [...]
> 
> AFAIK, as long as a reference exists on the stack, you should be safe,
> because the GC does scan the stack.  But if the C side stores it
> anywhere else past the point where the on-stack reference goes out of
> scope, then you're in trouble.

I see a problem here, in that the stack is changeable by the C function.

I can imagine a situation where it removes the item from the stack to put it somewhere else (e.g. global memory or on the C heap) *temporarily*, and by the time the function exits, the pointer is no longer in use in C-land. Thus, the function doesn't "save the pointer".

This kind of problem would still be susceptible to the GC collecting it (via another thread), and so I would recommend still storing locally in the calling function a copy of the pointer.

-Steve

On 11/16/20 1:25 PM, Steven Schveighoffer wrote:

> I can imagine a situation where it removes the item from the stack to
> put it somewhere else (e.g. global memory or on the C heap)
> *temporarily*

Good point. For example, they can provide the address of the member of a heap-allocated object:

  Obj * obj = malloc(/* ... */);
  D_function(&obj.member);
                               // <-- Another thread triggers a
                               //     GC collection at this point.

  printf("%d", obj.member);    // <-- Potentially too late.

This is hard to communicate to the callers. I said "safe to use for immediate consumption" but still not safe. Documenting like "pointer to stack allocated data only please" may be too confusing for casual users. So, the safest thing would be to make a copy with toStringz.

Of course, strings are just one type. I pass GC allocated large buffers to the C side without copying. Luckily, I am the only consumer so far and things seem to work. :)

Ali

On Monday, 16 November 2020 at 21:25:03 UTC, Steven Schveighoffer wrote:
> I can imagine a situation where it removes the item from the stack to put it somewhere else (e.g. global memory or on the C heap) *temporarily*, and by the time the function exits, the pointer is no longer in use in C-land. Thus, the function doesn't "save the pointer".
>
> This kind of problem would still be susceptible to the GC collecting it (via another thread), and so I would recommend still storing locally in the calling function a copy of the pointer.

How do you express that? LLVM can remove unused pointers? There are intrinsics to combat that, but is there something portable in D?