SUPPORT_DIRECT_MALLOC and SUPPORT_REMOVE_MALLOC_LEGACY

[dlmiles]

The idiom created is like so to allow potential instrumentation/debugging possibilities around specific sites if it is found specific memory bugs need chasing, maybe due to the scoping choice of this patch (the location of the init() and end() sites) is found to be incorrect.
It also marks with a unique grep'able term 'freeMagic1' to facilitate later cleanup/removal of the idiom, or inspection for optimization at a later date.
The '1' in the naming related to the deferred-by-one more than it being version 1.
There is some overlapping usage so the variable naming ended up as mm1 and mm1_ where a 2nd variable was needed.

While this patch seeks to not modify magic default configuration (you must opt-in via manual editing of defs.mak after ./configure) it is designed to allow community testing, bug hunting and reporting to take place.

PLEASE DO NOT ENABLE THESE FEATURE FLAGS FOR PRODUCTION USE, only evaluation and bug hunting.

TODO: The tcl version command (and the startup banner) needs indicate which feature flags are enable for the specific binary being run.

--

Methodology summary:
Changes applied in commit order (except for last commit).
Use CodeQL to find use-after-free sites.
Introduce freeMagic1 idiom at each callsite, with an assessment of scope (placement of init/end).
Create the last commit in the sequence based on the changes.

---

Comment taken from the lead commit:

This supports three build modes:

No additional -D options, default legacy mode

-DSUPPORT_DIRECT_MALLOC, magic will use direct calls to libc malloc/free
 and will leave in place the symbols now renamed as mallocMagicLegacy()
 freeMagicLegacy() and callocMagicLegacy().

-DSUPPORT_DIRECT_MALLOC -DSUPPORT_REMOVE_MALLOC_LEGACY as above but will
 remove the three legacy functions from the binary to provide assurance
 they can not be used.

The system malloc is thread-safe the legacy magic malloc has a global
deferred free pointer and the mmap() allocate has a free-list that is
not thread-safe making use of free not thread-safe.
This could of course be improved with the use of
atomic_compare_and_exchange operations but for what gain ?

Then there is the additional function call overhead (of the indirection)
and a few tests/branches inserted into a commonly used code paths around
memory allocation, it hides the call site of the malloc/free usage from
the compiler which maybe have special optimization cases.

The existing malloc/free makes static code analysis around memory
allocation more problematic, also use of runtime analysers will operate
better with a fail-fast to bad memory usage.

[RTimothyEdwards]

As I mentioned in another PR comment, I don't like the one-off deallocation but since it exists in ten thousand places in the code, I have just learned to live with it instead of trying to get rid of it. If this can be proven to catch all cases and not break anything anywhere, I'm perfectly fine to be done with the one-off deallocation, which has been, by and large, a royal pain in the neck.

[dlmiles]

I'll keep you posted, its too early to say. It is possible to apply this PR (build so libc malloc/free is used directly), then fire up magic under valgrind and do some things and then shutdown without any reported issues.
So if that is possible things can not be that bad. :)

It looks like the static code analysis worked well enough to immediately spot all the locations I was looking for first time.

I will take any pointers to ZIP projects and scripts to run to exercise things more thoroughly, I can run under valgrind/address-sanitizer/gdb over several days to find issues (or gain confidence).
Would definitely appreciate some kind of ZIP+scripts that demonstrate and exercise some of the lesser known features.
Maybe that would be good form of documentation for those features?
I am trying to create a project-of-projects that attempt code coverage.

[RTimothyEdwards]

If that were easy I would have created a comprehensive CI by now.
The feature set of magic is vast and it is a lot of work to exercise all the parts of the code, which covers everything from DRC and extraction to GDS generation to plotting and routing and interactive wiring. . .
I saw recently a video of a circuit being drawn that was related to Tiny Tapeout and likely (?) came from somebody using the (recently fixed) logcommands feature to save the entire layout process. Getting hold of such a command set would make a good coverage of interactive commands. For non-interactive stuff, running CACE on any of the sky130 analog blocks that support it (which is now most or all of them) would capture most of the features like DRC, extraction, and GDS generation, although (as I've pointed out before) some of the features like "plow" and "iroute" are so old that they are rarely used by anyone and hard to find examples for. The original magic tutorial set might be the best place to go to exercise those features.

[RTimothyEdwards]

Uri Shaked might know something about the magic layout video for TinyTapeout.

[dlmiles]

Merge Status: could be merged, but not expected to be merged, review this status in 2 months
Quality: usable as is (for example if memory bug chasing, this might provide a different angle and fail-fast to find the source of the bug easier)
Risk: very-high-impact, this may patch the 1st-order delayed-free use cases and appear to work, but I expect it will now uncover 2nd-order issues we don't know about yet
Level of Testing: valgrind with basic usage (extensive testing will take place over the coming months)
TODO: startup banner, --version, version command, should clearly indicate the feature flags a build was created with to help support issue around its use

It is expected that bugs found from it will be the subject of new/separate PRs, but it may depend on the bug.

[dlmiles]

1cad152 updated with 1st order issue CodeQL static analyser maybe not have spotted.

[dlmiles]

Rebase onto 8.3.545 no other changes

MacOS still builds successful but fails with Abort during runtime test. Maybe this is indicative of a double-free on exit

[dlmiles]

@RTimothyEdwards Looking to remove the draft status on this, opening this for release. Just need feedback this doesn't break the world for MacOS.

Recap:

• it seems the original commit was correct (after the efConnectionFreeLinkedList() mopup change)
• this does not change the default build (you have to opt-in manually after configure and read the comments in defs.mak to engage the malloc changes)
• the MacOS in CI shows startup failures (for some commands), which relate to Kick the Tyres testing not the building phase:
  dyld[25928]: missing symbol called is the unusual item seen in CI output, it is not clear what is missing

@d-m-bailey
Any Mac users with time to test compile, install and use to confirm this PR doesn't break normal usage.

# example of what the test compile setup looks like
cd /tmp
git clone https://github.com/RTimothyEdwards/magic
cd magic
git describe --long

git fetch origin pull/372/head:refs/heads/pull/372
git checkout pull/372
git describe

# follow your normal process here, maybe it looks like:
./scripts/configure_mac
make
make install

# the commands in CI showing error are the unusual sequence:
echo "=== magic -d null -noconsole -nowindow -T scmos"
echo "version ; quit" | magic -d null -noconsole -nowindow -T scmos
echo "=== magic -d null -noconsole -T scmos"
echo "version ; quit" | magic -d null -noconsole -T scmos

Then run it normally and see if it crashes on startup or exit.
The error dyld[25928]: missing symbol called seen in GitHub CI doesn't show any information to see the symbol, but this is expected on startup as it bails.
Then extended test to see if any MacOS specific issues under normal use.

[dlmiles]

Ok maybe the issue is due to:

#pragma weak symbol = symbol2

f173c0e#diff-8e0a99e32bba3bde3fac26382922326aa9403c7df62c359713e2ac034fca9243R182

Related LLVM/MacOS bug also:

• Weak aliases are not supported on MacOS X llvm/llvm-project#11488
• clang claims weak aliases are not supported on Darwin but only when using __attribute__, works fine with #pragma llvm/llvm-project#71001
• Weak aliases on MacOS aren't made global/public llvm/llvm-project#111321

[dlmiles]

Showing all green now, so maybe it was some interaction with building for DSO and weak symbols on MacOS.

So this is ready when everyone else is...