Merge pull request #348 from DataDog/jb/marking_fix

jbachorik · web-flow · commit ec1b4ce37550 · 2026-01-30T12:40:55.000+01:00
Add alignment checks to NativeFunc mark methods
diff --git a/.github/actions/upsert-pr-comment/action.yml b/.github/actions/upsert-pr-comment/action.yml
@@ -7,6 +7,10 @@ inputs:
   body-file:
     description: "Path to file whose contents become the comment body"
     required: true
+  comment-id:
+    description: "Unique identifier for this comment type (used to find/update existing comment)"
+    required: false
+    default: "upsert-pr-comment"
   repo:        # optional; defaults to triggering repo
     description: "Repository (owner/repo)."
     required: false
@@ -31,25 +35,30 @@ runs:
       env:
         GH_TOKEN: ${{ steps.octo-sts.outputs.token }}
         BODY_FILE: ${{ inputs['body-file'] }}
+        COMMENT_ID: ${{ inputs['comment-id'] }}
         REPO: ${{ inputs.repo || github.repository }}
         PR:   ${{ inputs['pr-number'] || github.event.pull_request.number }}
       shell: bash
       run: |
         if [[ -s "$BODY_FILE" ]]; then
           set -e
-          # find last comment by this actor
-          # first, build a jq filter that embeds the actor’s login
-          filter=".[] | select(.user.login == \"${GITHUB_ACTOR}\") | .id"
+          # Create marker to identify this comment type
+          MARKER="<!-- ${COMMENT_ID} -->"
+
+          # Find existing comment with this marker
           cid=$(gh api "repos/$REPO/issues/$PR/comments?per_page=100" \
-                --jq "${filter}" | tail -n1)
-  
+                --jq ".[] | select(.body | contains(\"${MARKER}\")) | .id" | head -n1)
+
+          # Prepend marker to body
+          BODY="${MARKER}"$'\n'"$(< "$BODY_FILE")"
+
           if [[ -n "$cid" ]]; then
             gh api --method PATCH "repos/$REPO/issues/comments/$cid" \
-              --raw-field body="$(< "$BODY_FILE")"
+              --raw-field body="$BODY"
             echo "✏️  Updated comment $cid"
           else
             gh api --method POST "repos/$REPO/issues/$PR/comments" \
-              --raw-field body="$(< "$BODY_FILE")"
+              --raw-field body="$BODY"
             echo "💬  Created new comment"
           fi
         else
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -11,6 +11,7 @@ on:
     tags-ignore:
       - v*
   pull_request:
+    types: [opened, synchronize, reopened, labeled]
   workflow_dispatch:
 
 permissions:
@@ -24,20 +25,18 @@ jobs:
     outputs:
       skip: ${{ steps.check.outputs.skip }}
     steps:
-      - name: Check if PR exists
+      - name: Check if PR exists for this branch (skip push if PR exists)
         id: check
         env:
           GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
-          HEAD_REF: ${{ github.head_ref }}
+          HEAD_REF: ${{ github.ref_name }}
         run: |
+          # On push events, base_ref is empty; skip if a PR already exists for this branch
           if [ -z "${{ github.base_ref }}" ]; then
             prs=$(gh pr list \
                 --repo "$GITHUB_REPOSITORY" \
-                --json baseRefName,headRefName \
-                --jq '
-                    map(select(.baseRefName == "${{ github.base_ref }}" and .headRefName == "$HEAD_REF}"))
-                    | length
-                ')
+                --json headRefName \
+                --jq "[.[] | select(.headRefName == env.HEAD_REF)] | length")
             if ((prs > 0)); then
                 echo "skip=true" >> "$GITHUB_OUTPUT"
             fi
@@ -116,19 +115,49 @@ jobs:
           # This ensures javadoc validation matches the exact conditions used during publishing
           ./gradlew :ddprof-lib:javadoc --no-daemon --parallel --build-cache --no-watch-fs
 
+  compute-configurations:
+    runs-on: ubuntu-latest
+    needs: check-for-pr
+    if: needs.check-for-pr.outputs.skip != 'true'
+    outputs:
+      configurations: ${{ steps.compute.outputs.configurations }}
+    steps:
+      - name: Debounce label events
+        if: github.event.action == 'labeled'
+        run: |
+          echo "Waiting 30s to allow adding multiple labels..."
+          sleep 30
+      - name: Compute test configurations from PR labels
+        id: compute
+        env:
+          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
+        run: |
+          # Always include debug
+          configs='["debug"'
+
+          # For PRs, check labels; for push/workflow_dispatch, use debug only
+          if [ "${{ github.event_name }}" = "pull_request" ]; then
+            labels=$(gh api repos/${{ github.repository }}/pulls/${{ github.event.pull_request.number }} --jq '.labels[].name' 2>/dev/null) || labels=""
+
+            if echo "$labels" | grep -Fq "test:release"; then
+              configs="$configs"',"release"'
+            fi
+            if echo "$labels" | grep -Fq "test:asan"; then
+              configs="$configs"',"asan"'
+            fi
+            if echo "$labels" | grep -Fq "test:tsan"; then
+              configs="$configs"',"tsan"'
+            fi
+          fi
+
+          configs="$configs]"
+          echo "configurations=$configs" >> $GITHUB_OUTPUT
+          echo "Test configurations: $configs"
+
   test-matrix:
-    needs: check-formatting
+    needs: [check-for-pr, check-formatting, compute-configurations]
     if: needs.check-for-pr.outputs.skip != 'true'
     uses: ./.github/workflows/test_workflow.yml
     with:
-      configuration: '["debug"]'
+      configuration: ${{ needs.compute-configurations.outputs.configurations }}
 
-  gh-release:
-    if: startsWith(github.event.ref, 'refs/heads/release/')
-    runs-on: ubuntu-latest
-    needs: [test-matrix]
-    steps:
-      - name: Create Github Release
-        uses: ./.github/workflows/gh_release.yml@gh-release
-        with:
-          release_branch: ${GITHUB_REF_NAME}
diff --git a/.github/workflows/codecheck.yml b/.github/workflows/codecheck.yml
@@ -52,6 +52,7 @@ jobs:
         uses: ./.github/actions/upsert-pr-comment
         with:
           body-file: comment.html
+          comment-id: scan-build-report
 
   codeql:
     if: needs.check-for-pr.outputs.skip != 'true'
diff --git a/ddprof-lib/src/main/cpp/codeCache.h b/ddprof-lib/src/main/cpp/codeCache.h
@@ -64,21 +64,40 @@ class NativeFunc {
   static void destroy(char *name);
 
   static short libIndex(const char *name) {
+    if (name == nullptr) {
+      return -1;
+    }
     NativeFunc* func = from(name);
     if (!is_aligned(func, sizeof(func))) {
       return -1;
     }
     return func->_lib_index;
   }
 
-  static bool isMarked(const char *name) { return from(name)->_mark != 0; }
+  static bool is_marked(const char *name) {
+    return read_mark(name) != 0;
+  }
 
-  static char mark(const char* name) {
-      return from(name)->_mark;
+  static char read_mark(const char* name) {
+    if (name == nullptr) {
+      return 0;
+    }
+    NativeFunc* func = from(name);
+    if (!is_aligned(func, sizeof(func))) {
+      return 0;
+    }
+    return func->_mark;
   }
 
-  static void mark(const char* name, char value) {
-      from(name)->_mark = value;
+  static void set_mark(const char* name, char value) {
+    if (name == nullptr) {
+      return;
+    }
+    NativeFunc* func = from(name);
+    if (!is_aligned(func, sizeof(func))) {
+      return;
+    }
+    func->_mark = value;
   }
 };
 
@@ -205,13 +224,13 @@ class CodeCache {
       for (int i = 0; i < _count; i++) {
           const char* blob_name = _blobs[i]._name;
           if (blob_name != NULL && predicate(blob_name)) {
-              NativeFunc::mark(blob_name, value);
+              NativeFunc::set_mark(blob_name, value);
           }
       }
 
       if (value == MARK_VM_RUNTIME && _name != NULL) {
           // In case a library has no debug symbols
-          NativeFunc::mark(_name, value);
+          NativeFunc::set_mark(_name, value);
       }
   }
 
diff --git a/ddprof-lib/src/main/cpp/profiler.cpp b/ddprof-lib/src/main/cpp/profiler.cpp
@@ -375,7 +375,7 @@ Profiler::NativeFrameResolution Profiler::resolveNativeFrameForWalkVM(uintptr_t
       // Get symbol name and check mark
       const char *method_name = nullptr;
       lib->binarySearch((void*)pc, &method_name);
-      char mark = (method_name != nullptr) ? NativeFunc::mark(method_name) : 0;
+      char mark = (method_name != nullptr) ? NativeFunc::read_mark(method_name) : 0;
 
       if (mark != 0) {
         return {nullptr, BCI_NATIVE_FRAME, true};  // Marked - stop processing
@@ -395,7 +395,7 @@ Profiler::NativeFrameResolution Profiler::resolveNativeFrameForWalkVM(uintptr_t
 
   // Fallback: Traditional symbol resolution
   const char *method_name = findNativeMethod((void*)pc);
-  if (method_name != nullptr && NativeFunc::isMarked(method_name)) {
+  if (method_name != nullptr && NativeFunc::is_marked(method_name)) {
     return {nullptr, BCI_NATIVE_FRAME, true};
   }
 
@@ -427,7 +427,7 @@ int Profiler::convertNativeTrace(int native_frames, const void **callchain,
         // binarySearch() returns symbol name, then we check mark (O(1))
         const char *method_name = nullptr;
         lib->binarySearch((void*)pc, &method_name);
-        char mark = (method_name != nullptr) ? NativeFunc::mark(method_name) : 0;
+        char mark = (method_name != nullptr) ? NativeFunc::read_mark(method_name) : 0;
 
         if (mark != 0) {
           // Terminate scan at marked frame
@@ -450,7 +450,7 @@ int Profiler::convertNativeTrace(int native_frames, const void **callchain,
 
     // Fallback: Traditional symbol resolution
     const char *method_name = findNativeMethod((void*)pc);
-    if (method_name != nullptr && NativeFunc::isMarked(method_name)) {
+    if (method_name != nullptr && NativeFunc::is_marked(method_name)) {
       // Terminate scan at marked frame
       return depth;
     }
diff --git a/ddprof-lib/src/main/cpp/stackWalker.cpp b/ddprof-lib/src/main/cpp/stackWalker.cpp
@@ -471,7 +471,7 @@ __attribute__((no_sanitize("address"))) int StackWalker::walkVM(void* ucontext,
             const char* method_name = (const char*)resolution.method_id;
             int frame_bci = resolution.bci;
             char mark;
-            if (frame_bci != BCI_NATIVE_FRAME_REMOTE && method_name != NULL && (mark = NativeFunc::mark(method_name)) != 0) {
+            if (frame_bci != BCI_NATIVE_FRAME_REMOTE && method_name != NULL && (mark = NativeFunc::read_mark(method_name)) != 0) {
                 if (mark == MARK_ASYNC_PROFILER && event_type == MALLOC_SAMPLE) {
                     // Skip all internal frames above malloc_hook functions, leave the hook itself
                     depth = 0;
@@ -516,7 +516,7 @@ __attribute__((no_sanitize("address"))) int StackWalker::walkVM(void* ucontext,
                     Profiler::NativeFrameResolution prev_resolution = profiler->resolveNativeFrameForWalkVM((uintptr_t)prev_native_pc, lock_index);
                     const char* prev_method_name = (const char*)prev_resolution.method_id;
                     if (prev_method_name != NULL) {
-                        char prev_mark = NativeFunc::mark(prev_method_name);
+                        char prev_mark = NativeFunc::read_mark(prev_method_name);
                         if (prev_mark == MARK_THREAD_ENTRY) {
                             // Thread entry point detected in previous frame (Rust thread_start, libc start_thread, etc.)
                             // This is the root frame - stop unwinding
diff --git a/ddprof-lib/src/main/cpp/utils.h b/ddprof-lib/src/main/cpp/utils.h
@@ -16,7 +16,7 @@ inline bool is_aligned(const T* ptr, size_t alignment) noexcept {
     auto iptr = reinterpret_cast<uintptr_t>(ptr);
 
     // Check if the integer value is a multiple of the alignment
-    return ((iptr & (~(alignment - 1))) == 0);
+    return ((iptr & (alignment - 1)) == 0);
 }
 
 inline size_t align_down(size_t size, size_t alignment) noexcept {
diff --git a/ddprof-lib/src/test/cpp/nativefunc_ut.cpp b/ddprof-lib/src/test/cpp/nativefunc_ut.cpp
diff --git a/ddprof-lib/src/test/cpp/utils_ut.cpp b/ddprof-lib/src/test/cpp/utils_ut.cpp
diff --git a/doc/RemoteSymbolication.md b/doc/RemoteSymbolication.md

Original file line number	Diff line number	Diff line change
`@@ -16,7 +16,7 @@ inline bool is_aligned(const T* ptr, size_t alignment) noexcept {`
`16`	`16`	`auto iptr = reinterpret_cast<uintptr_t>(ptr);`
`17`	`17`
`18`	`18`	`// Check if the integer value is a multiple of the alignment`
`19`		`- return ((iptr & (~(alignment - 1))) == 0);`
	`19`	`+ return ((iptr & (alignment - 1)) == 0);`
`20`	`20`	`}`
`21`	`21`
`22`	`22`	`inline size_t align_down(size_t size, size_t alignment) noexcept {`