[PROF-13123] Streamline upstream sources #333
Merged
+7,275
−835
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes the upstream async-profiler patching machinery and integrates the source code directly into the repository. The change simplifies source code management by eliminating the need to maintain patches and instead managing changes directly in source files.
Changes:
- Removed patching infrastructure including gradle tasks, configuration files, and lock files
- Moved async-profiler source files from
cpp-externaltocppdirectory - Updated build configurations to remove references to external source directories
Reviewed changes
Copilot reviewed 44 out of 45 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| gradle/patching.gradle | Deleted entire patching configuration system with 288 lines removed |
| gradle/lock.properties | Removed lock file tracking async-profiler branch and commit |
| doc/event-type-system.md | Updated path reference from cpp-external to cpp |
| ddprof-lib/src/test/make/Makefile | Removed cpp-external from source directories |
| ddprof-lib/src/test/fuzz/fuzz_dwarf.cpp | Updated comment removing async-profiler upstream reference |
| ddprof-lib/src/main/cpp/*.{h,cpp} | Added numerous async-profiler source files directly (vmStructs, symbols, stackFrame, stackWalker, os, etc.) |
| ddprof-lib/gtest/build.gradle | Removed cpp-external includes and dependencies |
| ddprof-lib/fuzz/build.gradle | Removed cpp-external includes and dependencies |
| ddprof-lib/build.gradle | Removed cloning, copying, and patching tasks; updated source paths |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+788
to
+799
| uintptr_t base_addr = (uintptr_t)base; | ||
| uint64_t symbol_value = sym->st_value; | ||
|
|
||
| // Skip this symbol if addition would overflow | ||
| // First check if symbol_value exceeds the address space | ||
| if (symbol_value > UINTPTR_MAX) { | ||
| continue; | ||
| } | ||
| // Then check if addition would overflow | ||
| if (base_addr > UINTPTR_MAX - (uintptr_t)symbol_value) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The overflow protection logic contains duplicated code with symbols_linux.cpp line 793. Consider extracting this pattern into a helper function to avoid code duplication and improve maintainability.
Comment on lines
+545
to
+552
| if (EMPTY_FRAME_SIZE > 0 || f.pc_off != DW_LINK_REGISTER) { | ||
| pc = stripPointer(*(void**)(sp + f.pc_off)); | ||
| } else if (depth == 1) { | ||
| pc = (const void*)frame.link(); | ||
| } else { | ||
| break; | ||
| } | ||
|
|
There was a problem hiding this comment.
Duplicated code block appears at lines 537-543 and 545-551. This duplication suggests potential copy-paste error or missing refactoring.
Suggested change
| if (EMPTY_FRAME_SIZE > 0 || f.pc_off != DW_LINK_REGISTER) { | |
| pc = stripPointer(*(void**)(sp + f.pc_off)); | |
| } else if (depth == 1) { | |
| pc = (const void*)frame.link(); | |
| } else { | |
| break; | |
| } |
Scan-Build Report
Bug Summary
Reports
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
3 tasks
Scan-Build Report
Bug Summary
Reports
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
jbachorik
reviewed
Jan 20, 2026
ddprof-lib/benchmarks/build.gradle
Outdated
| tasks.withType(CppCompile).configureEach { | ||
| dependsOn ':ddprof-lib:patchUpstreamFiles' | ||
|
|
||
| // TODO: Do we need this, or is this included by default? |
Collaborator
There was a problem hiding this comment.
I think the current state is ok. The remaining 'includes' will take care of the only source set we have
jbachorik
approved these changes
Jan 20, 2026
Collaborator
jbachorik
left a comment
jbachorik
left a comment
There was a problem hiding this comment.
Looking good. The copilot comments seem out of context, feel free to dismiss them.
As an idea for followup, you could create a Claude command in .claude/command that would take the brunt of checking the async-profiler upstream for any interesting/important modifications to the files we are using.
Scan-Build Report
Bug Summary
Reports
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?:
This change removes the patching machinery that we used to consume upstream async-profiler source code. Instead, we will maintain our changes directly in soure code going forward.
Motivation:
Additional Notes:
I copied the sources that have been generated in
src/main/cpp-externalintosrc/main/cpp, except forlog.handspinLock.hwhich have been present in both cases and where we need our version fromsrc/main/cpp. (Curiously, there has also been a duplicated filesymbols.h, where we apparently need the version from cpp-external.)I tried to remove everything that is related to the patching machinery, but might have missed some stuff.
How to test the change?:
Normal testing should not have any regressions.
For Datadog employees:
credentials of any kind, I've requested a review from
@DataDog/security-design-and-guidance.