Make sure we don't leak os.args. Fixes #1633.

[haesbaert]

os.args is never freed, while this is an insignificant leak, it is a bit annoying as it makes valgrind complain:

==234270== Memcheck, a memory error detector
==234270== Copyright (C) 2002-2024, and GNU GPL'd, by Julian Seward et al. ==234270== Using Valgrind-3.24.0 and LibVEX; rerun with -h for copyright info ==234270== Command: ./wc /tmp/mulumulu
==234270==
1 8 58 /tmp/mulumulu
==234270==
==234270== HEAP SUMMARY:
==234270== in use at exit: 47 bytes in 1 blocks
==234270== total heap usage: 5 allocs, 4 frees, 4,195,875 bytes allocated
==234270==
==234270== 47 bytes in 1 blocks are possibly lost in loss record 1 of 1
==234270== at 0x484BC13: calloc (vg_replace_malloc.c:1675)
==234270== by 0x402E49: runtime._heap_alloc-769 (in /d/learn-odin/wc/wc)
==234270== by 0x40A8D7: runtime.heap_alloc (in /d/learn-odin/wc/wc)
==234270== by 0x436E9D: runtime.heap_allocator_proc.aligned_alloc-0 (in /d/learn-odin/wc/wc)
==234270== by 0x4022DC: runtime.heap_allocator_proc (in /d/learn-odin/wc/wc)
==234270== by 0x4165E0: runtime.make_aligned-22560 (in /d/learn-odin/wc/wc)
==234270== by 0x41F6D0: runtime.make_slice-22340 (in /d/learn-odin/wc/wc)
==234270== by 0x40156B: os._alloc_command_line_arguments-4679 (in /d/learn-odin/wc/wc)
==234270== by 0x4011AF: __$startup_runtime (in /d/learn-odin/wc/wc)
==234270== by 0x406F17: main (in /d/learn-odin/wc/wc)
==234270==
==234270== LEAK SUMMARY:
==234270== definitely lost: 0 bytes in 0 blocks
==234270== indirectly lost: 0 bytes in 0 blocks
==234270== possibly lost: 47 bytes in 1 blocks
==234270== still reachable: 0 bytes in 0 blocks
==234270== suppressed: 0 bytes in 0 blocks
==234270==
==234270== For lists of detected and suppressed errors, rerun with: -s
==234270== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

With the fix the leak is gone, tested on linux only.

While here, also make _alloc_command_line_arguments() private.

--

full disclaimer: I'm pretty new to odin (second day), so double check the diff :-)

[harold-b]

Not freeing app-lifetime resident memory is a legitimate strategy and not a leak. Adding a superfluous free upon exit means adding extra labor for the app to perform and as such contributes to a slower exit.

I would find a way to appease valgrind manually instead.

[haesbaert]

Not freeing app-lifetime resident memory is a legitimate strategy and not a leak. Adding a superfluous free upon exit means adding extra labor for the app to perform and as such contributes to a slower exit.

I would find a way to appease valgrind manually instead.

I don't think it's superfluous if you're considering running tests under valgrind (or any other checker) and checking the exit code(--error-exitcode).

if the user wants to appease it in a portable manner, he would have to know the differences of how os.args is allocated (windows allocates per arg).

Not freeing program lifetime resident memory is a sound strategy, I agree, but I'd wager the decision should be left to the user, not pushed up by the runtime. Also see #1633 (comment)

As for the slower exit, we're talking about one free(3) on exit, which is nothing compared to how expensive exit(2) is, va/proc teardown and whatnot.

If this is unwanted, please feel free to drop it.

[odiferousmint]

I don't think that a single de-allocation has much if any overhead unless there are thousands (probably way more, on second thought) of command-line arguments. I agree that freeing is more ideal, as I don't think it affects performance, and it is cleaner.

Do the users have any control over it though? If so, then users could technically decide, but if not, then I think it would be a great addition.

Regardless, I think it is good practice to free memory one allocated.

As such, I don't think "it hinders performance" is a good argument against free, because that is extremely unlikely to happen, right? In fact, does it happen, ever, in the real-world or are we talking about hypothetical "slower exits"?

As for appeasing valgrind: that is more complicated to configure than adding a single free(), and that means EVERYONE has to do it, individually, if they don't want to see any warnings, which I don't. In fact, I prefer that valgrind tells me that there are no leaks when I'm writing C.

If there is a way to free it myself, in (or from) my own programs, let me know though.

By the way, regarding "slower" exits, only if anyone cared about performance as much as about "slower exits". Imagine how performant everything would be. :P But this is a topic for another time.

[harold-b]

As for the slower exit, we're talking about one free(3) on exit, which is nothing compared to how expensive exit(2) is, va/proc teardown and whatnot.

don't think that a single de-allocation has much if any overhead

The point is not a single deallocation having any effect, it obviously won't be noticeable. My point is the inclination of wanting to head that direction just because an external, completely unrelated tool dictates so when a something legitimate is already in place and you yourself admit:

Not freeing program lifetime resident memory is a sound strategy, I agree

Then I rest my case

but I'd wager the decision should be left to the user, not pushed up by the runtime

The user can also choose to get the args from the OS and not use the core procedures.

If this is unwanted, please feel free to drop it.

I've not authority, just a fellow community member. In any case, I'm not interesting in arguing over this, was just providing my viewpoint. Bill & Co. decide.

[odiferousmint]

The user can also choose to get the args from the OS and not use the core procedures.

Can you be so kind as to show me a snippet for both cases? I'm curious about the differences and maintainability. I might be "forced" to just suck it up if it can become a maintenance nightmare (or readability, whatever), but I'm not sure, I'd have to look at the code.

BTW, can't I just do defer on something myself AND use os.args or whatever?

[harold-b]

Can you be so kind as to show me a snippet for both cases? I'm curious about the differences and maintainability.

Sure, you actually don't have to go to the OS level with odin. The cstring version of the args are exposed publicly by the runtime, so just copy what its shown in the lines changed in the PR itself:

import "base:builtin"

main :: proc() {
    argv := make([]string, len(runtime.args__))
    for _, i in argv {
        argv[i] = string(runtime.args__[i])
    }
    defer delete(argv)
}

[odiferousmint]

Can you be so kind as to show me a snippet for both cases? I'm curious about the differences and maintainability.

Sure, you actually don't have to go to the OS level with odin. The cstring version of the args are exposed publicly by the runtime, so just copy what its shown in the lines changed in the PR itself:

import "base:builtin"

main :: proc() {
    argv := make([]string, len(runtime.args__))
    for _, i in argv {
        argv[i] = string(runtime.args__[i])
    }
    defer delete(argv)
}

Well, that doesn't look that bad I suppose. Although it'd be ideal if there wasn't two _s and didn't have to use runtime (I know I can import it with a shorter alias though), and if I didn't have to make and use string() but still.