Updated nif erlang process info#2374
Conversation
Signed-off-by: Mateusz Furga <mateusz.furga@swmansion.com>
| .. doxygenstruct:: BuiltInAtomRequestSignal | ||
| .. doxygenstruct:: ProcessInfoRequestSignal | ||
| :allow-dot-graphs: | ||
| .. doxygenstruct:: BuiltInAtomSignal |
| } | ||
| } // else: sender died | ||
|
|
||
| if (signal->len == 0) { |
There was a problem hiding this comment.
Let's make it an assert instead (or two, (signal->mode == PROCESS_INFO_LIST) and len > 0). nif_erlang_process_info doesn't send PROCESS_INFO_LIST if length is 0.
|
AMP, usual caveats: PR review:
|
The with_other_pid(Check) assertion was commented out in the original submission with no recorded reason; enabled it passes on both AtomVM and OTP (50/50 repeat runs) and it ran enabled with green CI in the FissionVM fork's merged copy of this feature. Temporary fixup commit: to be squashed into the process_info commit. Signed-off-by: Davide Bettio <davide@uninstall.it>
process_info/1 is by OTP definition one process_info/2 list call plus a presentation rule, so implement it in Erlang (maintainer preference: Erlang over C when OTP-compliant semantics allow). This deletes from C the default_items table, the argc == 1 branch, the third signal mode PROCESS_INFO_LIST_OMIT_UNREGISTERED and both registered_name filter sites; the signal protocol keeps a clean single/list request. The /1 default item set (OTP's default set intersected with AtomVM-supported items, in OTP's relative order) moves to erlang.erl, where extending it no longer requires a VM release. The erlang_tests runner resolves modules from its own directory and does not load atomvmlib, so the built estdlib erlang.beam (and its JIT-precompiled variant) is now copied next to the test beams; otherwise process_info/1 cannot be resolved by the test suite. Behaviorally verified equivalent on all probes, including cross-process /1, death races and 2000-item lists (found-issues.md section E). Note: inside the module named erlang, guard BIFs must be written qualified (erlang:is_pid/1), as unqualified calls resolve locally. Temporary fixup commit: to be squashed into the process_info commit. Signed-off-by: Davide Bettio <davide@uninstall.it>
term_is_pid also accepts external pids, whose boxed term was fed to term_to_local_process_id: an arbitrary process table lookup in release builds and an assert abort in debug builds. Match OTP behavior instead (verified on OTP 29): badarg for another node's pid, undefined for an external pid naming this node (a previous incarnation, so the process cannot be alive). Local pids are validated with VALIDATE_VALUE as process_flag/3 already does. Temporary fixup commit: to be squashed into the process_info commit. Signed-off-by: Davide Bettio <davide@uninstall.it>
OTP validates the info request argument before considering whether the target process is alive, so a dead pid combined with an invalid item, an invalid list element or an improper list is a badarg, not undefined (five empirically-diffed divergences vs OTP 29, one of them a regression vs the old AtomVM code, which checked term_is_atom before the process lookup). Item validity is probed with context_get_process_info(ctx, NULL, NULL, item, NULL), which returns pure membership and cannot drift from the supported item set. The list walk also counts the length, so the later copy loop no longer revalidates. The stale own-node external pid undefined answer moves after the argument validation for the same reason. Temporary fixup commit: to be squashed into the process_info commit. Signed-off-by: Davide Bettio <davide@uninstall.it>
The custom allocations-without-ensure-free CodeQL query (severity error) flags every context_get_process_info call in a NIF that is reachable before a memory_ensure_free barrier; the PR deleted the existing NOLINT suppression and the real CI raised two alerts on the size-pass call sites. Re-add the suppressions on the two size-pass sites and on the two new argument-validity probes, which the query reaches the same way. All four calls pass a NULL heap and cannot allocate. Temporary fixup commit: to be squashed into the process_info commit. Signed-off-by: Davide Bettio <davide@uninstall.it>
mailbox_send_process_info_request_signal returned void and silently dropped the request on malloc failure, while the NIF unconditionally set the Trap flag afterwards, leaving the caller waiting forever for an answer that would never come. The allocation is now caller-sized (list length), so this is easier to hit on memory-constrained targets. Return bool from the send function and raise out_of_memory synchronously instead of trapping; the list path also frees the items array on that branch. Temporary fixup commit: to be squashed into the process_info commit. Signed-off-by: Davide Bettio <davide@uninstall.it>
The get_item/2 helper asserted that the second process_info read returns exactly the value bound by the first, which for heap_size, total_heap_size and memory holds only while no GC lands between the two calls: on AtomVM each call runs memory_ensure_free_opt with MEMORY_CAN_SHRINK and on BEAM the first result allocation can trigger a minor GC, so the suite would pass almost always and then flake in CI. Volatile metrics now assert shape and invariants on both reads instead; exact double-read equality stays for stable items. The heap_size =< total_heap_size invariant is taken from one list request, which is an atomic snapshot. Temporary fixup commit: to be squashed into the process_info commit. Signed-off-by: Davide Bettio <davide@uninstall.it>
The old test asserted that the memory item grows when messages are queued (strictly on AtomVM, non-strictly on BEAM); the rewritten test_memory only checked is_integer, dropping the only functional check that memory responds to actual usage. Restore it inside the message_queue_len choreography, which already queues messages against a blocked helper process. Also add alive-cross-process invalid argument coverage to test_badargs: previously only self and dead pids were exercised, so error delivery for another live process was untested. Temporary fixup commit: to be squashed into the process_info commit. Signed-off-by: Davide Bettio <davide@uninstall.it>
Give process_info/1 its own edoc block (it previously sat under the process_info/2 doc comment), documenting the default key subset and the registered_name omission rule; extend the process_info/2 spec with the trap_exit item and the list-of-keys clause, and document both in the key list. Add a CHANGELOG note for the behavior change that ports are no longer accepted as process pids. Temporary fixup commit: to be squashed into the process_info commit. Signed-off-by: Davide Bettio <davide@uninstall.it>
A ProcessInfoRequestSignal processed from context_destroy was answered with a mid-destruction snapshot: at that point the process has already been removed from the process table and unregistered, but links and monitors are still attached, so the requester could observe a cleared registered_name for a process that is no longer lookupable. Erlang/OTP answers undefined for an exiting/released process. Reply undefined instead of gathering info when the request is processed from the destroy path, which is uniquely identified by process_table_locked: context_destroy is the only caller that holds the process table write lock. Requests sent after the requester observed a DOWN or an exit already returned undefined from the process table lookup, so this only changes the answer for requests racing with the teardown itself. Verified with a 2000-iteration spawn/die race in both single-item and list mode: no hangs, answers are either complete results or undefined. Signed-off-by: Davide Bettio <davide@uninstall.it>
context_message_queue_len used mailbox_len, which counts every node of both mailbox lists: unprocessed system signals sitting in the outer list (monitor and demonitor requests, unlink and group leader signals, process_info requests, ...) were reported as queued messages. A process whose true message queue was always empty could observe message_queue_len values in the thousands under signal load, while Erlang/OTP counts only ordinary messages; queue-length based logic such as load shedding or the proc_lib crash report code can act on wildly wrong values. Add mailbox_normal_message_len, which filters the outer list on NormalMessage exactly like mailbox_size already does (the inner list only holds normal messages), and use it for message_queue_len. The new monitor/demonitor flood test fails on the previous code within a few samples and must always report zero with the fix, on AtomVM and BEAM alike. The bug predates the process_info list support; it is fixed here because process_info/1 now bundles message_queue_len into every result, widening the exposure. Signed-off-by: Davide Bettio <davide@uninstall.it>
tests/erlang_tests runs against the bare VM with no library archive, by design; functions implemented in estdlib belong in tests/libs/ estdlib, which is exactly how the estdlib-implemented apply/2,3 are already covered there. Move the process_info/1 tests (default item set, cross-process, dead pid undefined, non-pid and foreign external pid badarg) into a new tests/libs/estdlib/test_process_info.erl - run on AtomVM via test_estdlib.avm and on BEAM by the estdlib-with-BEAM CI step - and revert the copying of the built estdlib erlang.beam into the erlang_tests runner directory, which worked against that design. The erlang_tests module keeps every native process_info/2 path; the new estdlib module additionally checks that a registered process reports registered_name first. Temporary fixup commit: to be squashed into the process_info commit. Signed-off-by: Davide Bettio <davide@uninstall.it>
nif_erlang_process_info answers [] before sending the signal, so a PROCESS_INFO_LIST request never carries an empty item list. Assert the two invariants the handler relies on instead of handling a length that cannot reach it. Signed-off-by: Davide Bettio <davide@uninstall.it>
ssize_t is POSIX and neither context.c nor nifs.c includes sys/types.h: it only resolves through transitive includes today. A size_t countdown loop needs no such type. Signed-off-by: Davide Bettio <davide@uninstall.it>
A request may repeat a dynamically sized item such as links or monitored_by, so the accumulated size is the request length times the per-item size and is not bounded by the item list itself. Allocating the heap multiplies that sum by sizeof(term), so the bound divides SIZE_MAX by it rather than comparing against SIZE_MAX. Cover the accumulation with a long repeated request, self and cross-process. Signed-off-by: Davide Bettio <davide@uninstall.it>
BuiltInAtomSignal was renamed to ImmediateSignal in a639cbc and the page was never updated, so the directive resolved to nothing. Restore the alphabetical order the process_info rename broke while here. Signed-off-by: Davide Bettio <davide@uninstall.it>
AtomsHashTable was removed in 2ab1acb ("Use valueshashtable for modules and remove atomshashtable"), but the libAtomVM API documentation still listed it, leaving a directive that resolves to nothing: a doxygen run over src/libAtomVM emits no such compound. Unrelated to process_info: found while fixing the neighbouring stale BuiltInAtomSignal entry pointed out in review. Signed-off-by: Davide Bettio <davide@uninstall.it>
ce3e299 to
712dd87
Compare
|
AMP keeps insisting on these, your call.. Follow-up review:
|
These changes are made under both the "Apache 2.0" and the "GNU Lesser General
Public License 2.1 or later" license terms (dual license).
SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later