On Mon, 14 Sep 2026 09:46:26 GMT, Andrew Haley <[email protected]> wrote:
>> This PR will strengthen the ability of `frame::safe_for_sender()` to >> validate an asynchronous sampled frame. Before this strengthen >> `frame::safe_for_sender()` could accept a faulty frame and later the >> `is_older()` check would cause an assertion to fail in `frame::sender()`. >> >> The added strengthening validates the sampled frame's sender FP, so >> `AsyncGetCallTrace()` can reject faulty frames. This was enough to prevent >> the assertion in my gprof-ng reproducer. >> >> However: Reading @caoman's comment in >> [JDK-8382486](https://bugs.openjdk.org/browse/JDK-8382486) I realized that >> the strengthening of `frame::safe_for_sender()` was not enough. A sender's >> FP that is within the stack and older than the current frame might slip >> through the strengthening of `frame::safe_for_sender()` but it might still >> point to something that is not a valid walkable stack frame. This is because >> a profiler samples the registers at an arbitrary point, which means that the >> id of a sampled frame might not be comparable with the current, so the >> `is_older()` assertion might still fail in `frame::sender()`. >> >> The solution was to mark all profiler/`AsyncGetCallTrace()` related register >> maps as async and treat them as unreliable in the `frame::sender()` assert. >> >> The strengthening of `frame::safe_for_sender()` is still in this PR because >> it's a good way for `AsyncGetCallTrace()` to reject faulty frames before >> calling `frame::sender()`. >> >> Unfortunately I have only been able to reproduce the failure by running >> `gprofng collect app` on a `x86` built java machine running the Derby test, >> but that has on the other hand been a very steady reproducer. So all changes >> in other platforms are just copies of the changes in `x86`. But since it's a >> generic code change I trust that it will work for all the other platforms as >> well. >> >> Passes tier1-3 on supported platforms. >> All other platforms (`riscv64`, `ppc64le` and `s390x`) has been tested with >> `TEST=serviceability/AsyncGetCallTrace/MyPackage/ASGCTBaseTest.java` using >> QEMU. >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > I'm not opposed to this patch as is, but it's another workaround for a broken > approach to unwinding. > DWARF CFI unwinding is the proper way to do it. > https://blog.mozilla.org/jseward/2014/05/13/lul-a-lightweight-unwinder-library-for-profiling-gecko/ > is worth investigating. > @theRealAph I'm not opposed to fixing this the proper way using DWARF CFI > unwinding, but that is a much bigger job. Until that is done, this PR fixes a > possible failing `assert()` when trying to profile using `gprofng`. I agree, but we're piling kludges on workarounds, building up technical debt. I have no objection to this specific patch, but one day we're going to have to pay back that debt. ------------- PR Comment: https://git.openjdk.org/jdk/pull/32850#issuecomment-5710385490
