Skip to content

Sanitizers#101

Merged
r1viollet merged 93 commits into
mainfrom
jb/asan
Jun 18, 2024
Merged

Sanitizers#101
r1viollet merged 93 commits into
mainfrom
jb/asan

Conversation

@r1viollet
Copy link
Copy Markdown
Contributor

@r1viollet r1viollet commented May 15, 2024

What does this PR do?:

Sanitizers

  • Add a configuration for sanitizers
  • Run TSan, UBsan, ASan
  • Ensure unit tests run with aligned configurations (and remove the CMake files)

Various TSan fixes

  • Adjust atomic operations to clarify some of the sequencing

Elf parser fixes

  • Fix to ignore some out of bound elf symbols
  • Addition of a test for the elf parser

CI

  • Daily report of sanitizer issues

Motivation:

Better static analysis

Additional Notes:

You can now run various build configurations through

./gradlew testasan
./gradlew testubsan
./gradlew testdebug
./gradlew testrelease

Additionally the library can be used to run on other types of workloads.

UBSAN_OPTIONS="halt_on_error=1 abort_on_error=1 print_stacktrace=1" LSAN_OPTIONS="detect_leaks=0" ASAN_OPTIONS="detect_stack_use_after_return=1" LD_PRELOAD=/usr/lib/gcc/x86_64-linux-gnu/11/libasan.so DD_SERVICE=test_r1 java -javaagent:./temp/dd-java-agent.jar -Ddd.profiling.enabled=true -Ddd.profiling.ddprof.enabled=true -Ddd.profiling.upload.period=10 -Ddd.profiling.start-force-first=true -Ddd.profiling.ddprof.debug.lib=/home/r1viollet/dd/java-profiler/ddprof-lib/build/lib/main/asan/linux/x64/libjavaProfiler.so -jar ./temp/renaissance-gpl-0.15.0.jar akka-uct -r 5

To avoid deadly signal issues on ubuntu 22, I had to run

sudo sysctl vm.mmap_rnd_bits=28

because of actions/runner-images#9491

How to test the change?:

This is run within our CI daily.

For Datadog employees:

  • If this PR touches code that signs or publishes builds or packages, or handles
    credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.
  • This PR doesn't touch any of that.

Unsure? Have a question? Request a review!

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 15, 2024

🔧 Report generated by pr-comment-scanbuild

Scan-Build Report

User:runner@fv-az585-289
Working Directory:/home/runner/work/java-profiler/java-profiler/ddprof-lib/src/test/make
Command Line:make -j4 clean all
Clang Version:Ubuntu clang version 14.0.0-1ubuntu1.1
Date:Tue Jun 18 08:33:18 2024

Bug Summary

Bug TypeQuantityDisplay?
All Bugs6
Logic error
Assigned value is garbage or undefined1
Dereference of null pointer3
Unused code
Dead initialization1
Dead nested assignment1

Reports

Bug Group Bug Type ▾ File Function/Method Line Path Length
Logic errorAssigned value is garbage or undefineddwarf.cppparseInstructions23520
Unused codeDead initializationlivenessTracker.cppcleanup_table451
Unused codeDead nested assignmentvmStructs.cppcheckNativeBinding8271
Logic errorDereference of null pointerflightRecorder.cppflush14058
Logic errorDereference of null pointersafeAccess.hload3518
Logic errorDereference of null pointersymbols_linux.hElfParser12832

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 15, 2024

🔧 Report generated by pr-comment-cppcheck

CppCheck Report

Warnings (5)

Style Violations (161)

r1viollet and others added 6 commits May 15, 2024 14:58
Ensure we have an aligned memory access to remove the asan warning
Ensure that we load the lock value in the loop
Comment thread gradle/configurations.gradle Outdated
}

def locateLibtsan() {
def libtsan_preinit = locate('libtsan_preinit.o')
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I do not require this to build locally. Is this required in CI ?

Comment thread ddprof-lib/src/main/cpp/profiler.cpp Outdated
@r1viollet r1viollet marked this pull request as ready for review June 13, 2024 12:52
@r1viollet r1viollet changed the title [Wip] Sanitizers Sanitizers Jun 13, 2024
Relax the ordering constraints
scenarios=$(cat ./artifacts/failures.txt | tr '\n' ',')
echo "Failed scenarios: $scenarios"

# curl -X POST "${{ secrets.SLACK_WEBHOOK }}" \
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Do we want to leave notifications ?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yes. Once we are ready to merge we should uncomment this block

table->_size = 0;
// The reset is not useful with the anon mmap setting the memory is zeroed.
// However this silences a false positive and should not have a performance impact.
table->clear();
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I could make this a conditional change. I do not think it matters in terms of performance, as it is a memset at startup and on growth.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Considering that the next sub-table is 2x size of the previous one the number of memset calls should not get high.

Copy link
Copy Markdown
Collaborator

@jbachorik jbachorik left a comment

Choose a reason for hiding this comment

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

LGTM

@r1viollet r1viollet merged commit 7627ee1 into main Jun 18, 2024
@github-actions github-actions Bot added this to the 1.9.0 milestone Jun 18, 2024
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.

3 participants