Skip to content

Comments

Address Sanitizer integration#212

Open
n1ywb-1 wants to merge 17 commits intojrincayc:masterfrom
n1ywb-1:asan
Open

Address Sanitizer integration#212
n1ywb-1 wants to merge 17 commits intojrincayc:masterfrom
n1ywb-1:asan

Conversation

@n1ywb-1
Copy link
Contributor

@n1ywb-1 n1ywb-1 commented Mar 24, 2025

This PR integrates address sanitizer, the most popular memory checker for C. All C projects should be using memory checking tools. ASAN is on par with valgrind, but much faster.

ASAN requires minor code changes for garbage collectors.

  1. We use the ASAN API to determine the real stack extents
  2. During the mark phase we use the ASAN API to detect and descend into "fake" stack frames on the heap so they can also be marked
  3. We poison free NODEs so ASAN can report illegal access to them

ASAN is only for debug builds due to added overhead, so these changes are guarded by #ifdef __SANITIZE_ADDRESS__.

I also threw in some GDB tools I created and fixed a couple of small bugs along the way.

n1ywb-1 added 11 commits March 24, 2025 18:05
Does a partial GC before constructing new case objects to ensure there are
enough free nodes to finish construction and add it to a root node
before the GC runs again.

Case objects must be marked twice so if the GC runs after a new case is
allocated but before it is added to the hash table AND a root AND the
pointer isn't found on the stack, eg because it was optimized out, then
the GC may free the cases.

I was concerned this would have a negative performance impact...

It actually improved unit test performance by 10-20%.

¯\_(ツ)_/¯
long int loses precision on 64 bit systems leading to possible crashing
This will help detect GC errors like use after free
Or use before alloc
@n1ywb-1
Copy link
Contributor Author

n1ywb-1 commented Mar 24, 2025

As a followon it would be good to run CI tests on an ASAN build as well as release

Comment on lines +29 to +33
set $NT_LIST=010000
set $NT_TREE=0100000
set $NT_AGGR=020000
set $NT_EMPTY=040000
set $NT_CASEOBJ=000001
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, these might be redundant since this is now part of the NodeTypes enum.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cut and pasted directly from the enum. They are redundant, but because these symbols are not defined in every scope GDB might break in, if the program is even still running (eg after a segfault), I came up with this.

There could be a better way, IDK. Probably not worth more effort unless you mean to use them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants