On Thursday, 22 October 2020 at 19:38:42 UTC, donallen wrote:
> On Thursday, 22 October 2020 at 18:54:24 UTC, tsbockman wrote:
>> On Thursday, 22 October 2020 at 04:02:10 UTC, donallen wrote:
>>> I tried this. With the gc on, the problem still occurs.
>>>
>>> I am working on 64-bit Arch Linux systems, up-to-date, DMD64 D Compiler v2.094.0
>>
>> Have you tried using a different compiler?
>
> Yes -- to no avail:
> dca@franz:~/Software/newcash_d/verifier$ make
> cd ../library ; make
> make[1]: Entering directory '/home/dca/Software/newcash_d/library'
> make[1]: Nothing to be done for 'all'.
> make[1]: Leaving directory '/home/dca/Software/newcash_d/library'
> #dmd -g -profile=gc -I=../library -L='-lsqlite3' verifier.d ../library/lib.o
> ldc -g -I=../library -L='-lsqlite3' verifier.d ../library/lib.o
> ../library/lib.o:lib.d:function _D3std9exception__T7bailOutHTC9ExceptionZQwFNaNfAyamMAxaZv: error: undefined reference to '_d_throwdwarf'
> ../library/lib.o:lib.d:function [...]

The errors you get are because you must compile *everything* with ldc (program name should be ldc2 BTW), but the lib.o file is still produced with dmd as it contains DMD specific symbols.

> I hate to say this, because so much of this project makes sense to me -- the design of the language, the effort that has been put into documentation, the helpfulness of the community -- but I think I have to throw in the towel. I can't spend any more time on this and it appears that D is just not solid enough for the work I'm trying to do with it.

Yeah, that's perfectly understandable.

As you've been said in one of the first answer this must be because D GC sees a pointer allocated with malloc, and this pointer looks orphan to the GC as it has no root.

Would you put something that's mallocated in a D struct or in a D array ?
From the memory point of view the code you have shown is correct (the `fromStringz().dup` is exactly what's must be done to prevent problems when mixin manual and D managed memory) so the origin of the problem really *has* to be somewhere else.

On Thu, Oct 22, 2020 at 10:05:42PM +0000, user1234 via Digitalmars-d wrote: [...]
> As you've been said in one of the first answer this must be because D GC sees a pointer allocated with malloc, and this pointer looks orphan to the GC as it has no root.

The GC does not collect objects allocated by malloc. (That would not make any sense, since if you allocated it with malloc, obviously you intend to deallocate it with free!)


T

-- 
I see that you JS got Bach.

On Thursday, 22 October 2020 at 19:34:18 UTC, donallen wrote:
>     struct Account
>     {
>         string name;
>         string path;
>         string guid;
>         string commodity_guid;
>         int flags;
>     }

Why did you call walk_account_tree on this, what does it do? Account has no pointers?

On Thursday, 22 October 2020 at 23:07:28 UTC, H. S. Teoh wrote:
> On Thu, Oct 22, 2020 at 10:05:42PM +0000, user1234 via Digitalmars-d wrote: [...]
>> As you've been said in one of the first answer this must be because D GC sees a pointer allocated with malloc, and this pointer looks orphan to the GC as it has no root.
>
> The GC does not collect objects allocated by malloc. (That would not make any sense, since if you allocated it with malloc, obviously you intend to deallocate it with free!)
>
>
> T

Yeah you're right. I was actually thinking to the opposite case:
the parent is mallocated but the members are not, eg

---
import core.memory;
import std.experimental.allocator;
import std.experimental.allocator.mallocator;
import std.experimental.allocator.common;

enum fill = "azertyuiopqsdfghjklm";

struct Node
{
    this(string c){content = c;}
    string content;
    Node*[] nodes;
}

void main()
{
    Node* root = make!Node(Mallocator.instance, fill);
    foreach(immutable i; 0 .. 10000)
    {
        root.nodes ~= make!Node(Mallocator.instance, fill);
        foreach(immutable j; 0 .. 100)
            root.nodes[i].nodes ~= make!Node(Mallocator.instance, fill);
    }
    assert(root.content == fill);
    foreach(immutable i; 0 .. root.nodes.length)
    {
        assert(root.nodes[i].content == fill);
        foreach(immutable j; 0 .. root.nodes[i].nodes.length)
            assert(root.nodes[i].nodes[j].content == fill);
    }
}
---

which crashes because of that (it "should" crash but I dont have much memory installed so maybe this is not the case for everyone)

On Thursday, 22 October 2020 at 22:05:42 UTC, user1234 wrote:
> On Thursday, 22 October 2020 at 19:38:42 UTC, donallen wrote:
>> On Thursday, 22 October 2020 at 18:54:24 UTC, tsbockman wrote:
>>> On Thursday, 22 October 2020 at 04:02:10 UTC, donallen wrote:
>>>> I tried this. With the gc on, the problem still occurs.
>>>>
>>>> I am working on 64-bit Arch Linux systems, up-to-date, DMD64 D Compiler v2.094.0
>>>
>>> Have you tried using a different compiler?
>>
>> Yes -- to no avail:
>> dca@franz:~/Software/newcash_d/verifier$ make
>> cd ../library ; make
>> make[1]: Entering directory '/home/dca/Software/newcash_d/library'
>> make[1]: Nothing to be done for 'all'.
>> make[1]: Leaving directory '/home/dca/Software/newcash_d/library'
>> #dmd -g -profile=gc -I=../library -L='-lsqlite3' verifier.d ../library/lib.o
>> ldc -g -I=../library -L='-lsqlite3' verifier.d ../library/lib.o
>> ../library/lib.o:lib.d:function _D3std9exception__T7bailOutHTC9ExceptionZQwFNaNfAyamMAxaZv: error: undefined reference to '_d_throwdwarf'
>> ../library/lib.o:lib.d:function [...]
>
> The errors you get are because you must compile *everything* with ldc (program name should be ldc2 BTW), but the lib.o file is still produced with dmd as it contains DMD specific symbols.

Yes, you are right. I deleted the .o files and rebuilt everything with ldc and it linked properly. But it not only produces the same error as dmd, but a new one as well (see the last line:

A commodity with the same name as the account does exist. The account will be linked to it.
 requires a link to a commodity but doesn't have one.
A commodity with the same name as the account does exist. The account will be linked to it.
 requires a link to a commodity but doesn't have one.
A commodity with the same name as the account does exist. The account will be linked to it.
 requires a link to a commodity but doesn't have one.
A commodity with the same name as the account does exist. The account will be linked to it.
 requires a link to a commodity but doesn't have one.
A commodity with the same name as the account does exist. The account will be linked to it.
 requires a link to a commodity but doesn't have one.
A commodity with the same name as the account does exist. The account will be linked to it.
 requires a link to a commodity but doesn't have one.
A commodity with the same name as the account does exist. The account will be linked to it.
walk_account_tree: child index exceeds n_children
dca@igor:~/Software/newcash_d/verifier$


>
>> I hate to say this, because so much of this project makes sense to me -- the design of the language, the effort that has been put into documentation, the helpfulness of the community -- but I think I have to throw in the towel. I can't spend any more time on this and it appears that D is just not solid enough for the work I'm trying to do with it.
>
> Yeah, that's perfectly understandable.
>
> As you've been said in one of the first answer this must be because D GC sees a pointer allocated with malloc, and this pointer looks orphan to the GC as it has no root.
>
> Would you put something that's mallocated in a D struct or in a D array ?
> From the memory point of view the code you have shown is correct (the `fromStringz().dup` is exactly what's must be done to prevent problems when mixin manual and D managed memory) so the origin of the problem really *has* to be somewhere else.

On 10/22/20 2:28 PM, donallen wrote:

> I've already provided valgrind output that shows the gc referencing an
> uninitialized variable.

I had similar problems. Luckily, all my problems were issues with my code.

* In one case, the program was linked with dmd but I was loading a shared library (written again in D) but I was calling dlopen(). Instead, I had to call Runtime.loadLibrary.

* In another case, my D library was being loaded by Python. I had forgotten to initialize the D runtime. I had to call Runtime.initialize.

* In yet another case, I was calling myObject.close() in destructors, forgetting that myObject was already finalized by the GC.

> Please understand -- I don't have infinite time to spend on this

It's some more work but are you aware of Dustmite, which may be able to magically reduce your issue to a minimal case?


https://dlang.org/blog/2020/04/13/dustmite-the-general-purpose-data-reduction-tool/

Ali

On Thursday, 22 October 2020 at 23:48:22 UTC, donallen wrote:
>>> I hate to say this, because so much of this project makes sense to me -- the design of the language, the effort that has been put into documentation, the helpfulness of the community -- but I think I have to throw in the towel. I can't spend any more time on this and it appears that D is just not solid enough for the work I'm trying to do with it.

BTW I forgot to mention, even if it's too late I suppose, that to convert a C program to D a way to go is:

1. convert minimally and use -betterC
2. leave the -betterC mode
3. incrementally add more idiomatic D feature

see https://dlang.org/blog/2018/06/11/dasbetterc-converting-make-c-to-d/ for more detail on the methodolgy.

On Thursday, 22 October 2020 at 21:58:04 UTC, rikki cattermole wrote:
> On 23/10/2020 4:56 AM, donallen wrote:
>> ==3854==    by 0x1B7E98: void verifier.main(immutable(char)[][]).walk_account_tree(verifier.main(immutable(char)[][]).Account, int) (verifier.d:272)
>> ==3854==  Uninitialised value was created by a stack allocation
>> ==3854==    at 0x1B77FC: void verifier.main(immutable(char)[][]).walk_account_tree(verifier.main(immutable(char)[][]).Account, int) (verifier.d:84)
>> ==3854==
>> ==3854== Conditional jump or move depends on uninitialised value(s)
>
> That is coming up an awful lot.
>
> We'd need walk_account_tree source, to know if its doing something funky.
>
> But upon reviewing how the entire thing is working, I think we need one_row and next_row_available_p source as well. Oh and reset_stmt too.

Here's the source code you requested:

    void walk_account_tree(Account account, int ancestor_flags)
    {
        void fix_missing_commodity()
        {
            immutable string new_commodity_guid = one_row(new_guid, &get_string).string_value;
            // Create new commodity
            bind_text(insert_new_commodity, 1, new_commodity_guid);
            bind_text(insert_new_commodity, 2, account.name);
            run_dm_stmt(insert_new_commodity, &reset_stmt);

            // And link the account to the new commodity
            bind_text(link_to_commodity, 1, new_commodity_guid);
            bind_text(link_to_commodity, 2, account.name);
            run_dm_stmt(link_to_commodity, &reset_stmt);
        }

        void check_and_repair_commodity_link()
        {
            if (account.commodity_guid.length == 0)
            {
                bind_text(get_possible_commodity_guid, 1, account.name);
                immutable Maybe maybe_commodity_guid = maybe_one_row(get_possible_commodity_guid,
                        &get_string);
                if (!maybe_commodity_guid.valid_p)
                {
                    // No commodity exists with the same name as the account.  Create one and link the account to it.
                    writeln(account.path,
                            " requires a link to a commodity but doesn't have one. A commodity
with the same name as the account does not exist. One will be created
with the symbol **unknown** and the account will be linked to it. If
you wish to get quotes for this commodity, you will need to fix the
symbol in Newcash.");
                    fix_missing_commodity();
                }
                else
                {
                    writeln(account.path, " requires a link to a commodity but doesn't have one.
A commodity with the same name as the account does exist. The account will be linked to it.");
                    bind_text(link_to_commodity, 1, maybe_commodity_guid.value.string_value);
                    bind_text(link_to_commodity, 2, account.guid);
                    run_dm_stmt(link_to_commodity, &reset_stmt);
                }
            }
            else
            {
                sqlite3_reset(get_possible_commodity_guid);
                /* This marketable account has a commodity. Check to be sure that the guid is valid.
           If not, set the account's commodity_guid to NULL and process this account again. */
                bind_text(verify_commodity_guid, 1, account.commodity_guid);
                Maybe maybe_valid_guid = maybe_one_row(verify_commodity_guid, &get_string);
                if (!maybe_valid_guid.valid_p)
                {
                    writeln(account.path,
                            " requires a commodity link and has one, but the commodity guid is
invalid. Creating a new commodity with the same name as the account and
linking the account to it.");
                    fix_missing_commodity();
                }
                else
                {
                    // Warn if the commodity name is not the same as the account name
                    bind_text(check_commodity_name, 1, account.guid);
                    MultiType n = one_row(check_commodity_name, &get_int);
                    if (n.integer_value == 0)
                    {
                        writeln(account.path, " requires a commodity link and has one, but the
commodity name is not the same as the account name. Is this intentional?");
                    }
                }
            }
        }

        // Placeholder?
        if ((account.flags & account_flag_placeholder) == 0)
        {
            // No. Is this account an asset?
            if ((ancestor_flags & account_flag_descendents_are_assets) != 0)
            {
                // Yes. Is it marketable?
                if ((ancestor_flags & account_flag_descendents_are_marketable) != 0)
                {
                    // Yes, account is a marketable asset. Check that it is linked to a proper commodity.
                    check_and_repair_commodity_link();
                }
                else
                {
                    // No
                    if (account.commodity_guid.length > 0)
                    {
                        // This account is a non-marketable asset and has a non-null commodity guid.
                        // Set to NULL. Non-marketable accounts should not point to commodities.
                        writeln(account.path,
                                " %s is a non-marketable asset, but it is associated with a
commodity. Removing the association by setting the account's commodity link to NULL.");
                        bind_text(nullify_commodity_guid, 1, account.guid);
                        run_dm_stmt(nullify_commodity_guid, &reset_stmt);
                    }
                    // Make sure the quantity is zero. Should not be otherwise for a non-marketable asset.
                    bind_text(check_quantities, 1, account.guid);
                    if (one_row(check_quantities, &get_int).integer_value > 0)
                    {
                        writeln(account.path,
                                " is a non-marketable asset account but has splits with non-zero quantities.
These will be fixed.");
                        bind_text(fix_quantities, 1, account.guid);
                        run_dm_stmt(fix_quantities, &reset_stmt);
                    }
                }
            }
            else
            {
                // This account is not an asset. Make sure it doesn't point to a commodity,
                // unless it is an Income account and inherits the descendents-need-commodity property.
                // Does it need a commodity link?
                if ((ancestor_flags & account_flag_descendents_need_commodity_link) != 0)
                {
                    // Yes. Is it an income account?
                    if ((ancestor_flags & account_flag_descendents_are_income) != 0)
                    {
                        // Yes
                        check_and_repair_commodity_link();
                    }
                    else
                    {
                        writeln("The account ", account.path,
                                " is not an Asset or Income account, but inherits the
'needs commodity' property. This should not be possible and
indicates a bug in Newcash or in the Verifier. Please report to Don Allen.");
                    }
                }
                else
                {
                    if (account.commodity_guid.length != 0)
                    {
                        // This account is not an asset, doesn't have the needs-commodity-link property and has a non-null commodity guid, which we set to NULL.
                        writeln(account.path, " is not an asset, doesn't have the needs-commodity-link property, but it is associated with a commodity.
Removing the association by setting the account's commodity link to NULL.");
                        bind_text(nullify_commodity_guid, 1, account.guid);
                        run_dm_stmt(nullify_commodity_guid, &reset_stmt);
                    }
                    // The account is not an asset and does not have needs-commodity-link property.
                    // Just make sure it doesn't inherit the marketable property
                    if ((ancestor_flags & account_flag_descendents_are_marketable) != 0)
                    {
                        writeln(account.path,
                                " is designated marketable, but is not an asset account. This
should not be possible and is indicative of a Newcash bug. Please report this to Don Allen.");
                    }
                }
                // Make sure the quantity is zero. Should not be otherwise for a non-asset.
                bind_text(check_quantities, 1, account.guid);
                int temp;
                if ((temp = one_row(check_quantities, &get_int).integer_value) > 0)
                {
                    writeln(account.path,
                            " %s is not an asset account but has splits with non-zero quantities. These will be fixed.");

                    bind_text(fix_quantities, 1, account.guid);
                    run_dm_stmt(fix_quantities, &reset_stmt);
                }
            }
        }
        else
        {
            // This account is a place-holder. Verify that it has no transactions. Warn the user if that is not true.
            bind_text(count_transactions, 1, account.guid);
            int transaction_count = one_row(count_transactions, &get_int).integer_value;
            if (transaction_count > 0)
            {
                writeln(account.path,
                        " is a placeholder account, but it has %d transactions. You should
re-assign them to an appropriate account with Newcash.");
            }
        }
        // Now do the children of this account
        // First determine how many there are
        bind_text(count_children, 1, account.guid);
        int n_children = one_row(count_children, &get_int).integer_value;
        if (n_children > 0)
        {
            Account[] children = new Account[n_children];
            bind_text(find_children, 1, account.guid);
            int i = 0;
            while (next_row_available_p(find_children, &reset_stmt))
            {
                if (i > n_children-1)
                    panic("walk_account_tree: child index exceeds n_children");
                children[i].name = fromStringz(sqlite3_column_text(find_children, 0)).dup;
                children[i].path = format("%s:%s", account.path, children[i].name);
                children[i].guid = fromStringz(sqlite3_column_text(find_children, 1)).dup; // guid
                children[i].commodity_guid = fromStringz(sqlite3_column_text(find_children, 2)).dup; // commodity_guid
                children[i].flags = sqlite3_column_int(find_children,
                        3) | account.flags & (account_flag_descendents_are_assets
                        | account_flag_descendents_are_liabilities | account_flag_descendents_are_income
                        | account_flag_descendents_are_expenses | account_flag_descendents_are_marketable
                        | account_flag_self_and_descendents_are_tax_related
                        | account_flag_descendents_need_commodity_link); // flags
                i = i + 1;
            }
            foreach (child; children) {
                walk_account_tree(child, ancestor_flags | account.flags);
            }
        }
    }

alias one_row_value_delegate = bool delegate (sqlite3_stmt* stmt, MultiType* result);
alias one_row_value_function = bool function (sqlite3_stmt* stmt, MultiType* result);
MultiType one_row(T)(sqlite3_stmt* stmt, T get_values)
{
    MultiType result;
    if (next_row_available_p(stmt, &reset_stmt))
    {
        if (get_values(stmt, &result))
        { // Did we get a value of the type requested?
            // Yes. Now make sure we only got one row back. This will also reset the stmt.
            if (next_row_available_p(stmt, &reset_stmt))
                panic("one_row: stmt returned more than one row");
            return result;
        }
        else
            panic("one_row: stmt returned a null value");
    }
    else
        panic("one_row: stmt returned no rows");
    return result; // Need this for compiler happiness -- can't get here
}

/// Check for availability of another row
bool next_row_available_p(sqlite3_stmt* stmt, int function(sqlite3_stmt* stmt) cleanup)
{
    switch (sqlite3_step(stmt))
    {
    case sqlite_row:
        return (true);
    case sqlite_done:
        /* Make the prepared stmt available for re-use. */
        cleanup(stmt);
        return (false);
    default:
        stderr.writeln("next_row_available_p failed",);
        stderr.writeln(sqlite3_errmsg(sqlite3_db_handle(stmt)), stderr);
        exit(EXIT_FAILURE);
    }
    return (false); // Need this for compiler happiness -- can't get here
}

/// Reset so statement can be re-used
int reset_stmt(sqlite3_stmt* stmt)
{
    return sqlite3_reset(stmt);
}

On Fri, Oct 23, 2020 at 12:47:11PM +0000, donallen via Digitalmars-d wrote:
> On Thursday, 22 October 2020 at 21:58:04 UTC, rikki cattermole wrote:
> > On 23/10/2020 4:56 AM, donallen wrote:
> > > ==3854==    by 0x1B7E98: void verifier.main(immutable(char)[][]).walk_account_tree(verifier.main(immutable(char)[][]).Account,
> > > int) (verifier.d:272)
> > > ==3854==  Uninitialised value was created by a stack allocation
> > > ==3854==    at 0x1B77FC: void verifier.main(immutable(char)[][]).walk_account_tree(verifier.main(immutable(char)[][]).Account,
> > > int) (verifier.d:84)
> > > ==3854==
> > > ==3854== Conditional jump or move depends on uninitialised value(s)
[...]

Hmm. Which lines in your posted code do lines 272 and 84 refer to? Those seem to be the problematic spots, but it's not clear where in the function they are, since your posted code doesn't indicate line numbers.


I also noticed this bit of code (which may or may not have anything to
do with it):

>                 int temp;
>                 if ((temp = one_row(check_quantities,
> &get_int).integer_value) > 0)
>                 {
>                     writeln(account.path,
>                             " %s is not an asset account but has splits with
> non-zero quantities. These will be fixed.");
> 
>                     bind_text(fix_quantities, 1, account.guid);
>                     run_dm_stmt(fix_quantities, &reset_stmt);
>                 }

The value of `temp` is not used after assignment. This is a rather odd way of writing it.  What's the reason you didn't just write:

	if (one_row(...) > 0)

instead?


[...]
> MultiType one_row(T)(sqlite3_stmt* stmt, T get_values)

What's the definition of MultiType?


[...]
>     return result; // Need this for compiler happiness -- can't get here

Note for the future: you could write `assert(0);` here to indicate that it should not be reachable. (It compiles to a single 'hlt' instruction, which will abort if the program somehow reaches it anyway.)


T

-- 
My program has no bugs! Only unintentional features...

On 10/23/20 8:47 AM, donallen wrote:
> On Thursday, 22 October 2020 at 21:58:04 UTC, rikki cattermole wrote:
>> On 23/10/2020 4:56 AM, donallen wrote:
>>> ==3854==    by 0x1B7E98: void verifier.main(immutable(char)[][]).walk_account_tree(verifier.main(immutable(char)[][]).Account, int) (verifier.d:272)
>>> ==3854==  Uninitialised value was created by a stack allocation
>>> ==3854==    at 0x1B77FC: void verifier.main(immutable(char)[][]).walk_account_tree(verifier.main(immutable(char)[][]).Account, int) (verifier.d:84)
>>> ==3854==
>>> ==3854== Conditional jump or move depends on uninitialised value(s)
>>
>> That is coming up an awful lot.
>>
>> We'd need walk_account_tree source, to know if its doing something funky.
>>
>> But upon reviewing how the entire thing is working, I think we need one_row and next_row_available_p source as well. Oh and reset_stmt too.
> 
> Here's the source code you requested:

This all looks ok. The only think I can possibly consider is that you are using bind_text a lot, to presumably bind sqlite prepared statements to D strings.

This means it's *possible* that the prepared statements have pointers to the strings, but nothing else does (hard to work it out from just reading the code, I can't tell when things go out of scope easily, and your prepared statements clearly exist outside these functions). If a prepared statement is a C-malloc'd item, then it's not scanned by the GC. This means that what possibly might be happening is that you bind a D string to a text parameter, the D string goes out of scope, the GC collects it and allocates it elsewhere, and then sqlite tries to use it to send some requests.

To test this theory, maybe try a version of the code that when calling bind_text, you append the string itself to a GC allocated array stored in a global. Might not prove anything, but if it's fixing the problem, maybe that's where the GC is collecting things that it shouldn't.

-Steve