unofficial mirror of bug-guile@gnu.org 
 help / color / mirror / Atom feed
* Fix for 1001-local-eval-error-backtrace-segfaults - please review
@ 2002-05-02 12:59 Neil Jerram
  2002-05-03  3:50 ` Thien-Thi Nguyen
  2002-05-07 18:36 ` Marius Vollmer
  0 siblings, 2 replies; 9+ messages in thread
From: Neil Jerram @ 2002-05-02 12:59 UTC (permalink / raw)
  Cc: M Johnson

Patch

--- /home/neil/Guile/1.6/guile-core/libguile/eval.c.old	Thu May  2 12:45:56 2002
+++ /home/neil/Guile/1.6/guile-core/libguile/eval.c	Thu May  2 12:46:21 2002
@@ -1417,7 +1417,9 @@
 	ls = scm_cons (scm_sym_define,
 		       z = scm_cons (n = SCM_CAR (x), SCM_UNSPECIFIED));
 	if (SCM_NNULLP (env))
-	  SCM_SETCAR (SCM_CAR (env), scm_cons (n, SCM_CAR (SCM_CAR (env))));
+	  env = scm_cons (scm_cons (scm_cons (n, SCM_CAAR (env)),
+				    SCM_CDAR (env)),
+			  SCM_CDR (env));
 	break;
       }
     case SCM_BIT8(SCM_MAKISYM (0)):

Diagnosis

If scm_unmemocopy is called (e.g. from scm_backtrace) to unmemoize an
expression that has an internal define (i.e. SCM_IM_DEFINE) near the
top level of the expression, the code in unmemocopy can modify the
expression passed in.  The modification is such that an extra copy of
the symbol being defined is added on every call, thus:

  (((args) 4) ...)
  (((xxx args) 4) ...)
  (((xxx xxx args) 4) ...)

and so on, and this modification eventually causes some other code
that looks at the environment to SEGV.

The copy in scm_unmemocopy, which looks as though it might be intended
to fix this problem, doesn't work because it only copies the
environment's top-level pair, and it is the car of the car of the
environment that gets modified as just described.

Fix notes

Basically, avoid modifying the environment in hand by making new list
structure instead.  This is similar to almost all the other cases in
unmemocopy, which use EXTEND_ENV.  Fix isn't very elegant, though;
is there a nicer way of doing this?

Checks

1. make check passes.

2. Rerun of problem scenarios:

guile> (define (fnc args) (local-eval '(define xxx 3) (the-environment)))
guile> (fnc 4)
standard input:1:33: In expression (define xxx 3):
standard input:1:33: Bad define placement
ABORT: (misc-error)

Type "(backtrace)" to get more information or "(debug)" to enter the debugger.
guile> (debug)
This is the Guile debugger; type "help" for help.
There are 3 frames on the stack.

Frame 2 at standard input:1:33
	(define xxx 3)
debug> eval (the-environment)
;value: (((args) 4) #<eval-closure 40262bd0>)
debug> ba
In standard input:
   2: 0* [fnc 4]
   1: 1  [local-eval (define xxx 3) (((args) 4) #<eval-closure 40262bd0>)]
   1: 2* (define xxx 3)
debug> eval (the-environment)
;value: (((args) 4) #<eval-closure 40262bd0>)
guile> 

guile> (load "/home/neil/segf.scm")
guile> (assignments (command-line))
<unnamed port>: In expression (define a (option-ref options b ...)):
<unnamed port>: Bad define placement
ABORT: (misc-error)

Type "(backtrace)" to get more information or "(debug)" to enter the debugger.
guile> (backtrace )

Backtrace:
In standard input:
   2: 0* [assignments ("/usr/local/bin/guile")]
In /home/neil/segf.scm:
   2: 1  (let ((env #)) (for-each (lambda # #) (quote #)))
   3: 2  [for-each #<procedure #f (triplet)> ((a b c) (d e f))]
In unknown file:
   ?: 3* [#<procedure #f (triplet)> (a b c)]
In /home/neil/segf.scm:
   5: 4* (let ((x #) (y #) (z #)) (local-eval (quasiquote #) env))
   8: 5  [local-eval (define a #) (# #)]
In unknown file:
   ?: 6* (define a (option-ref options b c))

Type "(debug-enable 'backtrace)" if you would like a backtrace
automatically if an error occurs in the future.
guile> 





_______________________________________________
Bug-guile mailing list
Bug-guile@gnu.org
http://mail.gnu.org/mailman/listinfo/bug-guile


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Fix for 1001-local-eval-error-backtrace-segfaults - please review
  2002-05-02 12:59 Fix for 1001-local-eval-error-backtrace-segfaults - please review Neil Jerram
@ 2002-05-03  3:50 ` Thien-Thi Nguyen
  2002-05-05 14:00   ` Neil Jerram
  2002-05-07 18:36 ` Marius Vollmer
  1 sibling, 1 reply; 9+ messages in thread
From: Thien-Thi Nguyen @ 2002-05-03  3:50 UTC (permalink / raw)
  Cc: bug-guile, johns776

   From: Neil Jerram <neil@ossau.uklinux.net>
   Date: 02 May 2002 13:59:06 +0100

   -	  SCM_SETCAR (SCM_CAR (env), scm_cons (n, SCM_CAR (SCM_CAR (env))));

   The copy in scm_unmemocopy, which looks as though it might be
   intended to fix this problem [...]

was this used previously?  (i'm trying to crawl inside the head of
whoever wrote it this way in the first place.)

   Fix isn't very elegant, though;
   is there a nicer way of doing this?

both the old way and the new way involve mutating some nested list
structure, so i'm guessing that doesn't play into "elegance".  i'm
wondering what is it about this fix that makes it not very elegant?

   2. Rerun of problem scenarios:

cool.  this touches upon the need to extend the testing framework to
handle interactive cases.  actually, i believe that's already possible
w/ the current framework via (ice-9 expect); the limitation really is
that all tests share an execution environment -- this is fine for the
most part, but undesirable for this kind of bug.

thi

_______________________________________________
Bug-guile mailing list
Bug-guile@gnu.org
http://mail.gnu.org/mailman/listinfo/bug-guile


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Fix for 1001-local-eval-error-backtrace-segfaults - please review
  2002-05-03  3:50 ` Thien-Thi Nguyen
@ 2002-05-05 14:00   ` Neil Jerram
  2002-05-15 12:43     ` Thien-Thi Nguyen
  0 siblings, 1 reply; 9+ messages in thread
From: Neil Jerram @ 2002-05-05 14:00 UTC (permalink / raw)
  Cc: bug-guile, johns776

>>>>> "thi" == Thien-Thi Nguyen <ttn@giblet.glug.org> writes:

    thi>    From: Neil Jerram <neil@ossau.uklinux.net>
    thi>    Date: 02 May 2002 13:59:06 +0100

    thi>    The copy in scm_unmemocopy, which looks as though it might be
    thi>    intended to fix this problem [...]

    thi> was this used previously?  (i'm trying to crawl inside the head of
    thi> whoever wrote it this way in the first place.)

I don't know.  The most likely ChangeLog entry I can find is `Tue Aug
20 18:48:40 1996 Mikael Djurfeldt', which describes the initial
addition of scm_unmemocopy.

    thi>    Fix isn't very elegant, though;
    thi>    is there a nicer way of doing this?

    thi> both the old way and the new way involve mutating some nested list
    thi> structure, so i'm guessing that doesn't play into "elegance".

No; the new way doesn't mutate at all.  It creates a new environment
that shares some substructure with the old environment.

    thi> i'm wondering what is it about this fix that makes it not
    thi> very elegant?

My fix may use more consing than it needs to.  Where possible, I feel
that mutation is desirable because it's faster and doesn't encourage
the GC.  So perhaps there's a fix that still works but with fewer than
3 new pairs.

    thi>    2. Rerun of problem scenarios:

    thi> cool.  this touches upon the need to extend the testing framework to
    thi> handle interactive cases.  actually, i believe that's already possible
    thi> w/ the current framework via (ice-9 expect); the limitation really is
    thi> that all tests share an execution environment -- this is fine for the
    thi> most part, but undesirable for this kind of bug.

But, does this test need a different execution environment?  It's true
for any existing test case that, if it causes a SEGV, the following
test cases won't run.  This doesn't worry us much because SEGVs aren't
an important part of our plans :-)

        Neil



_______________________________________________
Bug-guile mailing list
Bug-guile@gnu.org
http://mail.gnu.org/mailman/listinfo/bug-guile


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Fix for 1001-local-eval-error-backtrace-segfaults - please review
  2002-05-02 12:59 Fix for 1001-local-eval-error-backtrace-segfaults - please review Neil Jerram
  2002-05-03  3:50 ` Thien-Thi Nguyen
@ 2002-05-07 18:36 ` Marius Vollmer
  2002-05-07 19:30   ` Neil Jerram
  1 sibling, 1 reply; 9+ messages in thread
From: Marius Vollmer @ 2002-05-07 18:36 UTC (permalink / raw)
  Cc: Guile Bugs, M Johnson

Neil Jerram <neil@ossau.uklinux.net> writes:

> Basically, avoid modifying the environment in hand by making new list
> structure instead.  This is similar to almost all the other cases in
> unmemocopy, which use EXTEND_ENV.

Great!  (In case you are waiting for a go ahead: Go Ahead! :)

> Fix isn't very elegant, though; is there a nicer way of doing this?

I don't think we should worry too much about elegance in this part of
the code.

_______________________________________________
Bug-guile mailing list
Bug-guile@gnu.org
http://mail.gnu.org/mailman/listinfo/bug-guile


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Fix for 1001-local-eval-error-backtrace-segfaults - please review
  2002-05-07 18:36 ` Marius Vollmer
@ 2002-05-07 19:30   ` Neil Jerram
  2002-05-07 19:39     ` Marius Vollmer
  0 siblings, 1 reply; 9+ messages in thread
From: Neil Jerram @ 2002-05-07 19:30 UTC (permalink / raw)
  Cc: Guile Bugs

>>>>> "Marius" == Marius Vollmer <mvo@zagadka.ping.de> writes:

    Marius> Neil Jerram <neil@ossau.uklinux.net> writes:
    >> Basically, avoid modifying the environment in hand by making new list
    >> structure instead.  This is similar to almost all the other cases in
    >> unmemocopy, which use EXTEND_ENV.

    Marius> Great!  (In case you are waiting for a go ahead: Go Ahead! :)

    >> Fix isn't very elegant, though; is there a nicer way of doing this?

    Marius> I don't think we should worry too much about elegance in this part of
    Marius> the code.

OK.  Any thoughts on whether this is worth putting into the stable
branch as well?  I assume it's Rob's decision...

        Neil


_______________________________________________
Bug-guile mailing list
Bug-guile@gnu.org
http://mail.gnu.org/mailman/listinfo/bug-guile


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Fix for 1001-local-eval-error-backtrace-segfaults - please review
  2002-05-07 19:30   ` Neil Jerram
@ 2002-05-07 19:39     ` Marius Vollmer
  2002-05-11 15:52       ` Neil Jerram
  0 siblings, 1 reply; 9+ messages in thread
From: Marius Vollmer @ 2002-05-07 19:39 UTC (permalink / raw)
  Cc: Rob Browning, Guile Bugs

Neil Jerram <neil@ossau.uklinux.net> writes:

> OK.  Any thoughts on whether this is worth putting into the stable
> branch as well?  I assume it's Rob's decision...

If I understand the issue right (which I probably don't), the lossage
only occurs together with 'local-eval', right?

If that is the case, my opinion would be that it is not worth fixing
it in 'stable' and risking new obscure bugs.

_______________________________________________
Bug-guile mailing list
Bug-guile@gnu.org
http://mail.gnu.org/mailman/listinfo/bug-guile


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Fix for 1001-local-eval-error-backtrace-segfaults - please review
  2002-05-07 19:39     ` Marius Vollmer
@ 2002-05-11 15:52       ` Neil Jerram
  0 siblings, 0 replies; 9+ messages in thread
From: Neil Jerram @ 2002-05-11 15:52 UTC (permalink / raw)
  Cc: Rob Browning, Guile Bugs

>>>>> "Marius" == Marius Vollmer <mvo@zagadka.ping.de> writes:

    Marius> Neil Jerram <neil@ossau.uklinux.net> writes:
    >> OK.  Any thoughts on whether this is worth putting into the stable
    >> branch as well?  I assume it's Rob's decision...

    Marius> If I understand the issue right (which I probably don't), the lossage
    Marius> only occurs together with 'local-eval', right?

Well, I thought that I might be able to find another case just using
an internal define, but I haven't succeeded, so yes.

    Marius> If that is the case, my opinion would be that it is not worth fixing
    Marius> it in 'stable' and risking new obscure bugs.

OK, just unstable then.

        Neil


_______________________________________________
Bug-guile mailing list
Bug-guile@gnu.org
http://mail.gnu.org/mailman/listinfo/bug-guile


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Fix for 1001-local-eval-error-backtrace-segfaults - please review
  2002-05-05 14:00   ` Neil Jerram
@ 2002-05-15 12:43     ` Thien-Thi Nguyen
  2002-05-18 13:47       ` Neil Jerram
  0 siblings, 1 reply; 9+ messages in thread
From: Thien-Thi Nguyen @ 2002-05-15 12:43 UTC (permalink / raw)
  Cc: bug-guile, johns776

   From: Neil Jerram <neil@ossau.uklinux.net>
   Date: 05 May 2002 15:00:59 +0100

   No; the new way doesn't mutate at all.  It creates a new environment
   that shares some substructure with the old environment.

ok i see this now.

   But, does this test need a different execution environment?  It's true
   for any existing test case that, if it causes a SEGV, the following
   test cases won't run.  This doesn't worry us much because SEGVs aren't
   an important part of our plans :-)

sorry, "different" was unclear.  it needs to have an independent
execution environment, which may be the same in construction as for the
other tests, for precisely the reason you give.

thi

_______________________________________________
Bug-guile mailing list
Bug-guile@gnu.org
http://mail.gnu.org/mailman/listinfo/bug-guile


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Fix for 1001-local-eval-error-backtrace-segfaults - please review
  2002-05-15 12:43     ` Thien-Thi Nguyen
@ 2002-05-18 13:47       ` Neil Jerram
  0 siblings, 0 replies; 9+ messages in thread
From: Neil Jerram @ 2002-05-18 13:47 UTC (permalink / raw)
  Cc: bug-guile, johns776

>>>>> "thi" == Thien-Thi Nguyen <ttn@giblet.glug.org> writes:

    thi>    From: Neil Jerram <neil@ossau.uklinux.net>
    thi>    Date: 05 May 2002 15:00:59 +0100

    thi>    No; the new way doesn't mutate at all.  It creates a new
    thi>    environment that shares some substructure with the old
    thi>    environment.

    thi> ok i see this now.

However, I now wonder whether my non-mutating fix is OK.  After all,
it is important that a local define mutates the environment in which
it appears...  But perhaps this is sufficiently represented in C by
the reassignment of the `env' variable?

Any clarifying thoughts much appreciated.

        Neil


_______________________________________________
Bug-guile mailing list
Bug-guile@gnu.org
http://mail.gnu.org/mailman/listinfo/bug-guile


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2002-05-18 13:47 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2002-05-02 12:59 Fix for 1001-local-eval-error-backtrace-segfaults - please review Neil Jerram
2002-05-03  3:50 ` Thien-Thi Nguyen
2002-05-05 14:00   ` Neil Jerram
2002-05-15 12:43     ` Thien-Thi Nguyen
2002-05-18 13:47       ` Neil Jerram
2002-05-07 18:36 ` Marius Vollmer
2002-05-07 19:30   ` Neil Jerram
2002-05-07 19:39     ` Marius Vollmer
2002-05-11 15:52       ` Neil Jerram

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).