⚠️ `globalcontext_make_atom` is not safe

[bettio]

globalcontext_make_atom doesn't do any check and might return an invalid atom [1], so it will be changed to [2].
The reason is that globalcontext_make_atom may alloc memory (and it might fail in doing so), hence we need to handle it.

See also PR #1443

Handling this situation might complicate our codebase, so I suggest using some different approaches:

• globalcontext_existing_term_from_atom_string when we just need to compare a term against an ATOM_STR (question: should we rename this function to globalcontext_make_existing_atom?)
• interop_kv_get_value and interop_kv_get_value_default for fetching a value using an ATOM_STR as a key
• making atoms in advance, such as at the beginning of a function (when they are going to be used 100% of times), so in case of failure an early return is enough (I did a macro for that [3])
• adding most common ones to defaultatoms.def
• adding platform atoms to platform_defaultatoms.def when it doens't make sense to save few bytes of memory and they are frequently used (this might apply to emscripten)

Further evolution:

• defaultatoms located in src/libAtomVM/defaultatoms.def will be managed using gperf in the future (and therefore kept outside of the dynamic atom table in RAM), so it will be quite cheap adding new libAtomVM atoms there, since they will be located on flash memory. Also code generated with gperf is very efficient!
• I'm trying to understand how to add some verification code that makes sure that atom strings such as "\x9" "foo" will raise some kind of error (I'd love a build time error).

[1]:

417 static inline term globalcontext_make_atom(GlobalContext *glb, AtomString string)
418 {
419     int global_atom_index = globalcontext_insert_atom(glb, string);
420     return term_from_atom_index(global_atom_index);
421 }

[2]:

--- a/src/libAtomVM/globalcontext.h
+++ b/src/libAtomVM/globalcontext.h
@@ -412,11 +412,15 @@ static inline bool globalcontext_is_term_equal_to_atom_string(GlobalContext *glo
  *          global context.
  * @param glb pointer to the global context
  * @param string an AtomString
- * @return an atom term formed from the supplied atom string.
+ * @return an atom term formed from the supplied atom string, or an invalid term
  */
 static inline term globalcontext_make_atom(GlobalContext *glb, AtomString string)
 {
     int global_atom_index = globalcontext_insert_atom(glb, string);
+    if (UNLIKELY(global_atom_index < 0)) {
+        return term_invalid_term();
+    }
+
     return term_from_atom_index(global_atom_index);
 }

[3]:

#define REQUIRE_ATOM(name, lenstr, str, glb) \
    term name = globalcontext_make_atom(glb, lenstr str); \
    if (UNLIKELY(term_is_invalid_term(name))) { \
        RAISE_ERROR(OUT_OF_MEMORY_ATOM); \
    }

How to use it:

-static const char *const embedded_atom = "\x8" "embedded";
-
 static term nif_code_ensure_loaded(Context *ctx, int argc, term argv[])
 {
     UNUSED(argc);

+    REQUIRE_ATOM(embedded_atom, "\x8", "embedded", ctx->global);
+
     term module_atom = argv[0];
     if (UNLIKELY(!term_is_atom(module_atom))) {
         RAISE_ERROR(BADARG_ATOM);
@@ -4876,7 +4876,7 @@ static term nif_code_ensure_loaded(Context *ctx, int argc, term argv[])

     if (UNLIKELY(!found_module)) {
         term_put_tuple_element(result, 0, ERROR_ATOM);
-        term_put_tuple_element(result, 1, globalcontext_make_atom(ctx->global, embedded_atom));
+        term_put_tuple_element(result, 1, embedded_atom);
     } else {
         term_put_tuple_element(result, 0, MODULE_ATOM);
         term_put_tuple_element(result, 1, module_atom);

[UncleGrumpy]

While we are resolving this issue #1444 should also be fixed.

[UncleGrumpy]

• globalcontext_existing_term_from_atom_string when we just need to compare a term against an ATOM_STR (question: should we rename this function to globalcontext_make_existing_atom?)

This would help make the use of this function a lot more clear in my opinion.

[UncleGrumpy]

Some time ago we had discussed (in the Telegram chat) removing platform_defaultatoms entirely and delaying the creation of these atoms until a driver needs them, but this issue seems to complicate that slightly. Should we still be trying to migrate away from using platform_defaultatoms?

[jgonet]

• globalcontext_existing_term_from_atom_string when we just need to compare a term against an ATOM_STR (question: should we rename this function to globalcontext_make_existing_atom?)

globalcontext_make_existing_atom sounds better to me.

• making atoms in advance, such as at the beginning of a function (when they are going to be used 100% of times), so in case of failure an early return is enough (I did a macro for that [3])

Some guidelines when to create atoms dynamically and when to add it to the table would be helpful. If atom is used 100% of times then isn't it better to be created in table instead of dynamically? If atom is used e.g. only in error handling then creating it on a fly would be better (but it allocs)

Further evolution:

• I'm trying to understand how to add some verification code that makes sure that atom strings such as "\x9" "foo" will raise some kind of error (I'd love a build time error).

I think the easiest would be to make ATOM_STR assert on use (only in debug). Functions that deals with creation/use of atom strings should do that too.
If gperf allows iteration through keys, then maybe we can add a function that goes through them and checks it and call that function from CMake (check_c_source_compiles).