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

'eval' has problems, parse is not GC safe #9

Open
ghost opened this issue Oct 20, 2010 · 4 comments
Open

'eval' has problems, parse is not GC safe #9

ghost opened this issue Oct 20, 2010 · 4 comments
Assignees
Milestone

Comments

@ghost
Copy link

ghost commented Oct 20, 2010

In potion, eval has some problems that make it behave strangely sometimes, such as that loop: 'x = Object()' eval. segfaults while 'loop: x = Object().' eval doesn't.

I think this is due to the fact that potion_eval calls potion_parse which overwrites data in the P variable. One way to overcome this problem would be to create a new Potion object in potion_eval, but that would require using potion_create/potion_init, which overwrites global variables like PN_string that are vital to the operation of the original Potion object. The only way I can think of overcoming this is to use fork in potion_eval.

@rurban
Copy link
Member

rurban commented Sep 16, 2013

I tried to save and restore the overwritten P fields. parse should be re-entrant.
source, yypos, input. skip pbuf

But it turned out the problem is that PNSource in the parser is not GC safe. all the intermediate t and lhs objects
need to be moved also, and be checked for FWDs.

rurban pushed a commit that referenced this issue Sep 16, 2013
All the intermediate t and lhs objects in the compiler need to
be moved also while in a eval, and be checked for FWDs.
@rurban
Copy link
Member

rurban commented Sep 16, 2013

re-entrancy and testcase fixed with

commit d83d869
Author: Reini Urban [email protected]
Date: Mon Sep 16 12:32:35 2013 -0500

potion #9: fix parser is not re-entrant, but still not GC safe

All the intermediate t and lhs objects in the compiler need to
be moved also while in a eval, and be checked for FWDs.

rurban pushed a commit that referenced this issue Sep 17, 2013
@ghost ghost assigned rurban Oct 24, 2013
rurban pushed a commit that referenced this issue Jan 14, 2014
All the intermediate t and lhs objects in the compiler need to
be moved also while in a eval, and be checked for FWDs.
rurban pushed a commit that referenced this issue Jan 14, 2014
rurban pushed a commit that referenced this issue Jan 14, 2014
@robotii
Copy link
Contributor

robotii commented Feb 5, 2019

I had some luck with trying to make the parser GC safe. I can't guarantee that this is the right solution, but it seems to work for me.

void potion_gc_minor_parser(PN parser) {
  if(parser == 0)
    return;

  struct _GREG *G = (struct _GREG *)parser;
  struct PNMemory *M = ((Potion *)G->data)->mem;
  Potion *P = G->data;

  if(PN_IS_PTR(G->ss)) {
    GC_MINOR_UPDATE(G->ss);
    potion_mark_minor(G->data, (const struct PNObject *) G->ss);
  }
  if(PN_IS_PTR(G->val[0])) {
    GC_MINOR_UPDATE(G->val[0]);
    potion_mark_minor(G->data, (const struct PNObject *) G->val[0]);
  }
  int c = G->val - G->vals;
  for(int i = 0; i < c; i++) {
    if(PN_IS_PTR(G->vals[i])) {
      GC_MINOR_UPDATE(G->vals[i]);
      potion_mark_minor(G->data, (const struct PNObject *) G->vals[i]);
    }
  }
}

void potion_gc_major_parser(PN parser) {
  if(parser == 0)
    return;

  struct _GREG *G = (struct _GREG *)parser;
  struct PNMemory *M = ((Potion *)G->data)->mem;
  Potion *P = G->data;

  if(PN_IS_PTR(G->ss)) {
    GC_MAJOR_UPDATE(G->ss);
    potion_mark_major(P, (const struct PNObject *) G->ss);
  }
  if(PN_IS_PTR(G->val[0])) {
    GC_MAJOR_UPDATE(G->val[0]);
    potion_mark_major(P, (const struct PNObject *) G->val[0]);
  }
  int c = G->val - G->vals;
  for(int i = 0; i < c; i++) {
    if(G->vals[i] != NULL && PN_IS_PTR(G->vals[i])) {
      GC_MAJOR_UPDATE(G->vals[i]);
      potion_mark_major(P, (const struct PNObject *) G->vals[i]);
    }
  }
}

I made the calls right after the GC calls to the strings (for both major and minor GC)

    GC_MAJOR_STRINGS();
    potion_gc_major_parser(P->parser); // ensure the parser is GC safe

Sorry I don't have time to make a proper pull request, as my copy of potion has diverged from this repository, as well as a lack of time. I think this should help with making the parser a bit safer though.

@rurban
Copy link
Member

rurban commented Feb 5, 2019

Great, I'll look at it

rurban added a commit that referenced this issue Feb 6, 2019
move struct _GREG to greg.h, expose it in P as P->parser.
Idea by Peter Arthur
rurban added a commit that referenced this issue Feb 6, 2019
move struct _GREG to greg.h, expose it in P as P->parser.
Idea by Peter Arthur
rurban added a commit that referenced this issue Feb 6, 2019
move struct _GREG to greg.h, expose it in P as P->parser.
Idea by Peter Arthur
rurban added a commit that referenced this issue Dec 2, 2019
move struct _GREG to greg.h, expose it in P as P->parser.
Idea by Peter Arthur
rurban added a commit that referenced this issue Jan 3, 2023
move struct _GREG to greg.h, expose it in P as P->parser.
Idea by Peter Arthur
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants