Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Macros work by side effect and assume function's parameter is named root #19

Open
NJdevPro opened this issue Nov 12, 2024 · 0 comments
Open

Comments

@NJdevPro
Copy link

NJdevPro commented Nov 12, 2024

The macro ADD_ROOT works only if the first parameter of the functions is named root.

This is especially not good, because the root parameter shadows the root global variable !

  extern void *root;    // Global variable
  
  void some_function(void *root) {  // Parameter shadows global
      // Creates local array and links it
      ADD_ROOT(2);  
      // ... uses local 'root' parameter
  }

The thing is, if we rename the global variable into say gc_root, or the parameter root into say *input, the code may still compile (due to shadowing) and even work for some time and start to crash in random ways.
And yet, the compiler will NOT warn about the shadowing with -Wall (you need -Wshadow).
And it will NOT warn about unused parameter input either.
And the current code works not despite the shadowing but BECAUSE of the shadowing !
This has caused memory corruption and many hours of debugging.

Here is a fix:

  static inline void** add_root_frame(void **prev_frame, int size, void *frame[]) {
      frame[0] = prev_frame;
      for (int i = 1; i <= size; i++)
          frame[i] = NULL;
      frame[size + 1] = ROOT_END;
      return frame;
  }
  
  #define DEFINE1(prev_frame, var1)                \
      void *root_frame[3];                         \
      prev_frame= add_root_frame(prev_frame, 1, root_frame);  \
      Obj **var1 = (Obj **)(root_frame + 1);
  
  #define DEFINE2(prev_frame, var1, var2)          \
      void *root_frame[4];                         \
      prev_frame= add_root_frame(prev_frame, 2, root_frame);  \
      Obj **var1 = (Obj **)(root_frame + 1);       \
      Obj **var2 = (Obj **)(root_frame + 2);
  
  #define DEFINE3(prev_frame, var1, var2, var3)   \
      void *root_frame[5];                        \
      prev_frame= add_root_frame(prev_frame, 3, root_frame); \
      Obj **var1 = (Obj **)(root_frame + 1);      \
      Obj **var2 = (Obj **)(root_frame + 2);      \
      Obj **var3 = (Obj **)(root_frame + 3);
  
  #define DEFINE4(prev_frame, var1, var2, var3, var4)   \
      void *root_frame[6];                        \
      prev_frame= add_root_frame(prev_frame, 4, root_frame); \
      Obj **var1 = (Obj **)(root_frame + 1);      \
      Obj **var2 = (Obj **)(root_frame + 2);      \
      Obj **var3 = (Obj **)(root_frame + 3);      \
      Obj **var4 = (Obj **)(root_frame + 4);

Now we have to add root to every DEFINE, but at least there is no bad surprise.

static void add_variable(void *root, Obj **env, Obj **sym, Obj **val) {
DEFINE2(root, vars, tmp);
...
}

The dependency of DEFINE2 on root is explicit. Now we can rename the global variable gc_root without issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant