On Saturday, 5 January 2019 at 12:14:15 UTC, Russel Winder wrote:
> Indeed. I should do that to see if I can reproduce the problem to submit a proper bug report.
>
> File_Ptr is wrapping a dvb_file * from libdvbv5 to try and make things a bit for D and to ensure RAII. libdvbv5 is a C API with classic C approach to handling objects and data structures.
>
> My DStep/with manual binding is at https://github.com/russel/libdvbv5_d and the application using it (which is causing the problems) is at https://github.com/russel/DVBTune

Your problem possibly (probably?) stems from

auto channelsData = TransmitterData(args[1]).scan(frontendId);

The temporary TransmitterData(args[1]) is, well, temporary and its destructor runs after that expression is done. As the returned object from scan references data from the temporary, you have a stale pointer.

> I have a feeling that I am really not doing things in a D idiomatic way.

Some driveby style comments then:

>	bool opEquals()(const FrontendId other) const {
>		if (this is other) return true;
>		if (other is null) return false;
>		return this.adapter_number == other.adapter_number && this.frontend_number == other.frontend_number;
>	}

The compiler generated default opEquals will do basically the same thing. Ditto for the other types. You usually want to take a const ref for opEquals since there is no point copying it.

> if (other is null)

I'm surprised the compiler doesn't warn or error on that as the only way that could make sense would be if it had an alias this to a pointer type.

You should consider reference counting your pointer wrapper types, FrontendParameters_Ptr/File_Ptr/ScanHandler_Ptr

You seem to like const, good! You don't need to take `const int`s as parameters, you're getting a copy anyway. You have a bunch of redundant casts as well.

I'll have another looks tomorrow when I'm a bit more awake.

On Sat, 2019-01-05 at 13:14 +0000, Nicholas Wilson via Digitalmars-d-learn wrote:
> 
[…]
> Your problem possibly (probably?) stems from
> 
> auto channelsData = TransmitterData(args[1]).scan(frontendId);
> 
> The temporary TransmitterData(args[1]) is, well, temporary and its destructor runs after that expression is done. As the returned object from scan references data from the temporary, you have a stale pointer.

Actually that is not a worry since the TransmitterData instance is only needed to call the scan function which creates a ChannelsData instance that holds no references to the TransmitterData instance.

It turns out that whilst the code used to run, and now doesn't, all the things
we have been talking of are nothing to do with the core problem. It turns out
that the function call to initialise the File_Ptr object is returning a valid
object with invalid data. Thus the unknown change is likely in the libdvbv5
library either due to the C API doing different things or the adapter created
using DStep doing the wrong thing.

> > I have a feeling that I am really not doing things in a D idiomatic way.
> 
> Some driveby style comments then:
> 
> > 	bool opEquals()(const FrontendId other) const {
> > 		if (this is other) return true;
> > 		if (other is null) return false;
> > 		return this.adapter_number == other.adapter_number &&
> > this.frontend_number == other.frontend_number;
> > 	}
> 
> The compiler generated default opEquals will do basically the same thing. Ditto for the other types. You usually want to take a const ref for opEquals since there is no point copying it.

I deleted them, added a test or three (should have done this ages ago) and the tests pass. So teh generated methods do the needful. Thanks for that "heads up".

> > if (other is null)
> 
> I'm surprised the compiler doesn't warn or error on that as the only way that could make sense would be if it had an alias this to a pointer type.
> 
> You should consider reference counting your pointer wrapper types, FrontendParameters_Ptr/File_Ptr/ScanHandler_Ptr

Very true. For now I have forbidden copying, which is wrong for this sort of thing. If D had the equivalent of C++ std::shared_ptr or Rust std::rc::Rc or std::rc::Arc, that would be the way forward But I guess having explicit reference counting is not too hard.

> You seem to like const, good! You don't need to take `const int`s as parameters, you're getting a copy anyway. You have a bunch of redundant casts as well.

I am a person who always makes Java variables final, and loves that Rust variables are immutable by default!

-- 
Russel.
===========================================
Dr Russel Winder      t: +44 20 7585 2200
41 Buckmaster Road    m: +44 7770 465 077
London SW11 1EN, UK   w: www.russel.org.uk

On Tuesday, 8 January 2019 at 10:23:30 UTC, Russel Winder wrote:
> Actually that is not a worry since the TransmitterData instance is only needed to call the scan function which creates a ChannelsData instance that holds no references to the TransmitterData instance.
>
> It turns out that whilst the code used to run, and now doesn't, all the things
> we have been talking of are nothing to do with the core problem. It turns out
> that the function call to initialise the File_Ptr object is returning a valid
> object with invalid data. Thus the unknown change is likely in the libdvbv5
> library either due to the C API doing different things or the adapter created
> using DStep doing the wrong thing.

Ahh. Good that you've found that, I can't help you much more with that then.

>> The compiler generated default opEquals will do basically the same thing. Ditto for the other types. You usually want to take a const ref for opEquals since there is no point copying it.
>
> I deleted them, added a test or three (should have done this ages ago) and the tests pass. So teh generated methods do the needful. Thanks for that "heads up".

No problems, less code is good code.

> Very true. For now I have forbidden copying, which is wrong for this sort of thing. If D had the equivalent of C++ std::shared_ptr or Rust std::rc::Rc or std::rc::Arc, that would be the way forward But I guess having explicit reference counting is not too hard.

I believe Andrei and Razvan are working on that, part of that being the Copy Constructor DIP. Hopefully it will arrive soon.

>> You seem to like const, good! You don't need to take `const int`s as parameters, you're getting a copy anyway. You have a bunch of redundant casts as well.
>
> I am a person who always makes Java variables final, and loves that Rust variables are immutable by default!

Indeed, less to think about is always nice.

Good luck figuring out why your data is dud.
Nic

On 1/5/19 6:33 AM, Russel Winder wrote:
> On Sat, 2019-01-05 at 10:52 +0000, Russel Winder wrote:
>> On Sat, 2019-01-05 at 10:31 +0000, Nicholas Wilson via Digitalmars-d-learn
>> wrote:
>> […]
>>> Maybe it is a problem with copying a File_Ptr (e.g. missing a
>>> increase of the reference count)? Like, `auto a = File_Ptr(); {
>>> auto b = a; }` and b calls the destructor on scope exit.
>>> That would be consistent with having problems copying to object
>>> to pass to writeln.
>>
>> I found the problem and then two minutes later read your email and bingo we
>> have found the problem.
>>
>> Previously I had used File_Ptr* and on this occasion I was using File_Ptr
>> and
>> there was no copy constructor because I have @disable this(this). Except
>> that
>> clearly copying a value is not copying a value in this case. Clearly this
>> situation is what is causing the destructor to be called on an unconstructed
>> value. But I have no idea why.
>>
>> The question now, of course, is should I have been using File_Ptr instead of
>> File_Ptr* in the first place. I am beginning to think I should have been.
>> More
>> thinking needed.
> 
> Switching to using File_Ptr* I now get the SIGSEGV at the end of main as you
> were thinking before. Oh f###.
> 
> This code used to work. :-(
> 

Russel, make sure your destructor both checks whether the underlying resource is set, and clears it to invalid when freeing it.

Even types that can't be copied can be moved, or temporarily created as rvalues. When they are moved the shell they get moved out of is still destructed! So it has to have a state where it can be destroyed, even though there is no resource.

Maybe some inspiration here: https://github.com/MartinNowak/io/blob/master/src/std/io/file.d#L189-L196

-Steve

On Tue, 2019-01-08 at 09:59 -0500, Steven Schveighoffer via Digitalmars-d- learn wrote:
> 
[…]
> 
> Russel, make sure your destructor both checks whether the underlying resource is set, and clears it to invalid when freeing it.
> 
> Even types that can't be copied can be moved, or temporarily created as rvalues. When they are moved the shell they get moved out of is still destructed! So it has to have a state where it can be destroyed, even though there is no resource.

I have added tests in the destructor but given the constructor should throw an exception on a failure to initialise the internal state correctly, it really ought to be unnecessary. but I guess it cant hurt being there!

As I noted to Nicholas it seems the application is getting a valid data structure returned with invalid data and that is where the SIGSEGV is. This is really weird as I have just finished a Rust version of the same application and it works fine. And this D version used to work fine. It is a real mystery why there is a problem now.

Sadly the D plugin to CLion doesn't as yet have the same functionality as the Rust plugin.  Debugging these sorts of thing is just so much better in CLion than trying to work GDB manually.

> Maybe some inspiration here: https://github.com/MartinNowak/io/blob/master/src/std/io/file.d#L189-L196
> 

I will check that out, thanks for the pointer.

-- 
Russel.
===========================================
Dr Russel Winder      t: +44 20 7585 2200
41 Buckmaster Road    m: +44 7770 465 077
London SW11 1EN, UK   w: www.russel.org.uk

On Tue, 2019-01-08 at 11:51 +0000, Nicholas Wilson via Digitalmars-d-learn wrote:
> 
[…]
> Ahh. Good that you've found that, I can't help you much more with that then.

Indeed. :-)

Your hep to get to this point though has been invaluable. Thanks you for putting in the time and effort.

[…]

> Good luck figuring out why your data is dud.

It really is totally weird. My new Rust binding to libdvbv5 and associated version of the same application works fine. So libdvbv5 itself is not the cuprit. This has to mean it is something about the D compilers that has changed the way the D binding to libdvbv5 behaves.

If only the D plugin to CLion were much further down the road this would be much easier to fix. I had an issue in the Rust and it was fixed in a couple of minutes because of the way CLion drives GDB for you. Using GDB manually is such a f###### pain. This alone becomes a mountain that leads to the thought of giving up on the D version.

In an ideal world JetBrains would take over the D plugin, but that isn't gong
to happen – unlike what happened for Go and Rust. What the D plugin needs is
some full time workers: the great work by the current volunteers is slow
progress by nature of it being volunteer effort by a few people.

-- 
Russel.
===========================================
Dr Russel Winder      t: +44 20 7585 2200
41 Buckmaster Road    m: +44 7770 465 077
London SW11 1EN, UK   w: www.russel.org.uk

On Wednesday, 9 January 2019 at 16:48:47 UTC, Russel Winder wrote:
> On Tue, 2019-01-08 at 11:51 +0000, Nicholas Wilson via Digitalmars-d-learn wrote:
>> [...]
> […]
>> [...]
>
> [...]

If debugger integration is that important to you, you might want to try out visual studio code with the corresponding plugins (you need a separate plugin for debugger support). I found it to work quite decently.

On Wed, 2019-01-09 at 20:03 +0000, Johannes Loher via Digitalmars-d-learn wrote:
> 
[…]
> If debugger integration is that important to you, you might want to try out visual studio code with the corresponding plugins (you need a separate plugin for debugger support). I found it to work quite decently.

Or I could stop doing D and Rust programming and switch to Kotlin and Java and get stuck into helping the IntelliJ-DLanguage team as I said I would a couple of years ago.

-- 
Russel.
===========================================
Dr Russel Winder      t: +44 20 7585 2200
41 Buckmaster Road    m: +44 7770 465 077
London SW11 1EN, UK   w: www.russel.org.uk

On Wednesday, 9 January 2019 at 16:48:47 UTC, Russel Winder wrote:
> It really is totally weird. My new Rust binding to libdvbv5 and associated version of the same application works fine. So libdvbv5 itself is not the cuprit. This has to mean it is something about the D compilers that has changed the way the D binding to libdvbv5 behaves.

Hmm, if you think the binding could be the problem you could try using app as an alternative, see if it makes any difference.

On 1/9/19 11:39 AM, Russel Winder wrote:
> On Tue, 2019-01-08 at 09:59 -0500, Steven Schveighoffer via Digitalmars-d-
> learn wrote:
>>
> […]
>>
>> Russel, make sure your destructor both checks whether the underlying
>> resource is set, and clears it to invalid when freeing it.
>>
>> Even types that can't be copied can be moved, or temporarily created as
>> rvalues. When they are moved the shell they get moved out of is still
>> destructed! So it has to have a state where it can be destroyed, even
>> though there is no resource.
> 
> I have added tests in the destructor but given the constructor should throw an
> exception on a failure to initialise the internal state correctly, it really
> ought to be unnecessary. but I guess it cant hurt being there!

The point is that some libraries are not robust enough to handle freeing data multiple times. And with the way postblit/dtors work, you have to handle this properly in D.

> As I noted to Nicholas it seems the application is getting a valid data
> structure returned with invalid data and that is where the SIGSEGV is. This is
> really weird as I have just finished a Rust version of the same application
> and it works fine. And this D version used to work fine. It is a real mystery
> why there is a problem now.

Hm... your description of having the problem happen at the end of main seems to suggest it has something to do with destruction.

-Steve