Fun/Crashes with newer GCC on Debian

Hi,

I looked into Paolo's patches and ran make check to see a crash in
the pools.st. But it turns out it is crashing due me upgrading to
GCC 4.8/Binutils 2.23.52.20130828-1  (something in the layout
of the text changed and I see a new class of bugs).

I disabled the incremental and generational GC and used the address
sanitizer (I wondered if that is a good idea) and found some issues.

pools.st:
UndefinedObject>>#executeStatements.

The code assume that _gst_curr_method->v_method.currentClass is a
valid class. It is NIL though. E.g. in instance_variable_index the
call to  _gst_instance_variable_array will actually access garbage
(and it started to crash for me).

Same goes for _gst_verify_method and CLASS_FIXED_FIELDS. I added
IS_NIL checks to make the asan report go away. I have no idea if
that is the right thing.


exceptions.st:

NIL is casted to a method context in disable_non_unwind_contexts.
I have added this. Judging from the loop above this patch, I assume
that newContextOOP will in deed be NIL at the bottom of the stack.

@@ -1232,13 +1232,15 @@ disable_non_unwind_contexts (OOP returnContextOOP)
     }
 
   /* Skip any disabled methods.  */
-  while UNCOMMON (CONTEXT_FLAGS (newContext)
-                  == (MCF_IS_METHOD_CONTEXT | MCF_IS_DISABLED_CONTEXT))
+  while UNCOMMON (/*!IS_NIL(newContextOOP) &&*/ (CONTEXT_FLAGS (newContext)
+                  == (MCF_IS_METHOD_CONTEXT | MCF_IS_DISABLED_CONTEXT)))
     {
       oldContext = newContext;
 
       /* Descend in the chain...  */
       newContextOOP = oldContext->parentContext;
+      if (IS_NIL(newContextOOP))
+       break;
       newContext = (gst_method_context) OOP_TO_OBJ (newContextOOP);
 
       /* This context cannot be deallocated in a LIFO way.  We must


C-code:

I get asan reports in _gst_grey_oop_range *page = *page;. With
generations off and NO_INCREMENTAL_GC set. Can't this be a NO-OP?


thanks
        holger

_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

On Fri, Oct 11, 2013 at 08:08:57AM +0200, Holger Hans Peter Freyther wrote:
> Hi,
>
> I looked into Paolo's patches and ran make check to see a crash in
> the pools.st. But it turns out it is crashing due me upgrading to
> GCC 4.8/Binutils 2.23.52.20130828-1  (something in the layout
> of the text changed and I see a new class of bugs).

and I forgot the issue I have difficulties to understand.

compiler.st is reporting an asan error. So in frame 3 the
"startPos - in_stream->fileOffset" will be -6 and p - 6
will not point to the right place. Any idea?

holger


Breakpoint 1, 0xb69ea850 in __asan_report_error () from /usr/lib/i386-linux-gnu/libasan.so.0
(gdb) frame 2
#2  0xb6848e81 in _gst_counted_string_new (s=s@entry=0xb5c0efda "", len=len@entry=12)
    at dict.c:1976
1976    memcpy (string->chars, s, len);
(gdb) frame 1
#1  0xb69e38cf in __asan_report_load1 () from /usr/lib/i386-linux-gnu/libasan.so.0
(gdb) frame 2
#2  0xb6848e81 in _gst_counted_string_new (s=s@entry=0xb5c0efda "", len=len@entry=12)
    at dict.c:1976
1976    memcpy (string->chars, s, len);
(gdb) frame 3
#3  0xb689c04c in _gst_get_source_string (startPos=14, endPos=26) at input.c:548
548    result = _gst_counted_string_new (p + (startPos - in_stream->fileOffset),
(gdb) p *in_stream
$1 = {type = STREAM_OOP, pushedBackChars = " \000", pushedBackCount = 0, line = 1,
  column = 29, prompt = 0x0, fileOOP = 0x4042b800,
  fileName = 0xb697e600 "a Smalltalk Stream", fileOffset = 20, st = {u_st_file = {
      fd = 1078402496, buf = 0xb5c0efe0 "printNl", ptr = 0xb5c0efe7 "", end = 0xb5c0efe7 ""},
    u_st_str = {strBase = 0x40471dc0 "0鮴", str = 0xb5c0efe0 "printNl"}, u_st_oop = {
      oop = 0x40471dc0, buf = 0xb5c0efe0 "printNl", ptr = 0xb5c0efe7 "",
      end = 0xb5c0efe7 ""}}, prevStream = 0xb5601760}
(gdb) p in_stream->fileOffset
$2 = 20
(gdb) p startPos
$3 = 14
(gdb) p p
$4 = 0xb5c0efe0 "printNl"
(gdb) q


_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

On Fri, Oct 11, 2013 at 08:08:57AM +0200, Holger Hans Peter Freyther wrote:

> pools.st:
> UndefinedObject>>#executeStatements.


diff --git a/libgst/gst-parse.c b/libgst/gst-parse.c
index 1ad0512..91cb03a 100644
--- a/libgst/gst-parse.c
+++ b/libgst/gst-parse.c
@@ -1961,7 +1961,10 @@ parse_compile_time_constant (gst_parser *p)
 
   return _gst_make_method (&location, loc(p, 0),
                            NULL, temps, NULL, statements, NULL,
-                           _gst_current_parser->currentClass,
+                           IS_NIL(_gst_current_parser->currentClass)
+                             ?
+                               _gst_undefined_object_class :
+                               _gst_current_parser->currentClass,
                            _gst_nil_oop,
                            false);
 }

 [
   ##(Exception printNl)
 ]



I need to return.. maybe return false? I don't know...


_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

Il 11/10/2013 15:08, Holger Hans Peter Freyther ha scritto:
compile_compile_time_constant always uses nil as the receiver.
I think you should:

(1) in parse_compile_time_constant, change the class to the metaclass
for _gst_current_parser->currentClass

(2) in compile_compile_time_constant, change the receiver to the
instance class of the method's class (aka METACLASS_INSTANCE).

Yeah, return false and set *chain = _gst_nil_oop too.

Paolo

>
> _______________________________________________
> help-smalltalk mailing list
> [hidden email]
> https://lists.gnu.org/mailman/listinfo/help-smalltalk
>


_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

Il 11/10/2013 08:08, Holger Hans Peter Freyther ha scritto:
> C-code:
>
> I get asan reports in _gst_grey_oop_range *page = *page;. With
> generations off and NO_INCREMENTAL_GC set. Can't this be a NO-OP?

This is done to generate a segfault, but only if the page is still
unwritable.  It is definitely a false positive.

Paolo

_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

Il 11/10/2013 08:25, Holger Hans Peter Freyther ha scritto:
It's a lookahead problem, here is a reduced testcase

st> Object extend [ x [ ^thisContext parentContext method methodSourceString ] ]
st> '''abc'' printNl ''def'' x printNl' readStream fileIn
'abc'
'x printN'
a ReadStream

Paolo


_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

On Sat, Oct 12, 2013 at 06:45:05PM +0200, Paolo Bonzini wrote:
> Il 11/10/2013 08:08, Holger Hans Peter Freyther ha scritto:
> > C-code:
> >
> > I get asan reports in _gst_grey_oop_range *page = *page;. With
> > generations off and NO_INCREMENTAL_GC set. Can't this be a NO-OP?
>
> This is done to generate a segfault, but only if the page is still
> unwritable.  It is definitely a false positive.

Okay, but with NO_SIGSEGV_HANDLING defined we could avoid this. The
pages are readable/writable anyway so will not generate a segfault
while read/written?

diff --git a/libgst/oop.c b/libgst/oop.c
index c8af8c6..71e837a 100644
--- a/libgst/oop.c
+++ b/libgst/oop.c
@@ -1630,6 +1630,7 @@ tenure_one_object ()
 void
 _gst_grey_oop_range (PTR from, size_t size)
 {
+#ifndef NO_SIGSEGV_HANDLING
   volatile char *last, *page;
 
   for (last = ((char *)from) + size,
@@ -1637,6 +1638,7 @@ _gst_grey_oop_range (PTR from, size_t size)
        page < last;
        page += getpagesize())
     *page = *page;
+#endif
 }


holger

_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

On Sat, Oct 12, 2013 at 06:13:46PM +0200, Paolo Bonzini wrote:

Eval [
  ##(Exception printNl)
]

the currentClass is nil in this case? So you ask for using
 "nil class asMetaclass"

> Yeah, return false and set *chain = _gst_nil_oop too.

great, I will do the change and push it to master.

holger

_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

Il 13/10/2013 13:58, Holger Hans Peter Freyther ha scritto:
It should not be nil, it should be UndefinedObject (so you use
UndefinedObject asMetaclass).

Paolo

_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

Il 12/10/2013 19:11, Holger Hans Peter Freyther ha scritto:
Yes, that's correct.

Paolo


_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

On Sun, Oct 13, 2013 at 03:12:14PM +0200, Paolo Bonzini wrote:
> > the currentClass is nil in this case? So you ask for using
> >  "nil class asMetaclass"
>
> It should not be nil, it should be UndefinedObject (so you use
> UndefinedObject asMetaclass).

I think there is a difference between DoIt and Eval. During the
begin of the eval the code expects:

   assert (IS_NIL (_gst_current_parser->currentClass));

the DoIt might temporarily set things to the UndefinedObject
and after conditionally calls _gst_reset_compilation_category.


So my current code is this. It is fixing my segfault. When I
set _gst_current_parser->currentClass I would need to reset
it to _gst_nil_oop when executed during the Eval.

comments?


@@ -1947,6 +1949,7 @@ static tree_node
 parse_compile_time_constant (gst_parser *p)
 {
   tree_node temps, statements;
+  OOP currentClass;
   YYLTYPE location = *loc(p,0);
 
   assert (token (p, 0) == '#');
@@ -1959,9 +1962,14 @@ parse_compile_time_constant (gst_parser *p)
   if (!statements || _gst_had_error)
     return _gst_make_oop_constant (&location, _gst_nil_oop);
 
+  currentClass = _gst_current_parser->currentClass;
+  if (IS_NIL(_gst_current_parser->currentClass))
+    currentClass = _gst_undefined_object_class;
+//  _gst_current_parser->currentClass = currentClass;
+
   return _gst_make_method (&location, loc(p, 0),
                            NULL, temps, NULL, statements, NULL,
-                           _gst_current_parser->currentClass,
+                           currentClass,
                            _gst_nil_oop,
                            false);
 }


_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

A compile time constant is being compiled outside of a class. At the
same time currentClass has been casted to a class. This means that some
random memory has been accessed. For compile time constants check if
the current class isNil (e.g. during an eval) and then start to use
"nil class class" which should be equivalent to "nil class asMetaclass".

2013-11-30  Holger Hans Peter Freyther  <[hidden email]>

        * gst-parse.c: Check if currentClass is nil before calling
        _gst_make_method.
---
 libgst/gst-parse.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/libgst/gst-parse.c b/libgst/gst-parse.c
index 232e172..bd551d8 100644
--- a/libgst/gst-parse.c
+++ b/libgst/gst-parse.c
@@ -1948,6 +1948,7 @@ static tree_node
 parse_compile_time_constant (gst_parser *p)
 {
   tree_node temps, statements;
+  OOP currentClass;
   YYLTYPE location = *loc(p,0);
 
   assert (token (p, 0) == '#');
@@ -1960,9 +1961,14 @@ parse_compile_time_constant (gst_parser *p)
   if (!statements || _gst_had_error)
     return _gst_make_oop_constant (&location, _gst_nil_oop);
 
+  /* This can happen when parsing an Eval statement */
+  currentClass = _gst_current_parser->currentClass;
+  if (IS_NIL(_gst_current_parser->currentClass))
+    currentClass = OOP_CLASS(_gst_undefined_object_class);
+
   return _gst_make_method (&location, loc(p, 0),
                            NULL, temps, NULL, statements, NULL,
-                           _gst_current_parser->currentClass,
+                           currentClass,
                            _gst_nil_oop,
                            false);
 }
--
1.8.4.rc3


_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

On Sun, Oct 13, 2013 at 03:12:45PM +0200, Paolo Bonzini wrote:

> Yes, that's correct.

I have pushed this and the other ASAN issue with compile time
constants. I will look into getting an ASAN build on travis-ci
as well.


_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk