vishapoberon / compiler

vishap oberon compiler
http://oberon.vishap.am
GNU General Public License v3.0
186 stars 25 forks source link

Bug of memory access after disposing it #39

Closed Oleg-N-Cher closed 7 years ago

Oleg-N-Cher commented 7 years ago

Dear David.

Once you've got rid of alloca in favor of an explicit allocation and deallocation, there was a problem that still doesn't solved, only masked.

    PROCEDURE Prefixed(x: OPT.ConstExt;  y: ARRAY OF CHAR): BOOLEAN;
        VAR i: INTEGER;
    BEGIN i := 0; 
        WHILE x[i+1] = y[i] DO INC(i) END ;
        RETURN y[i] = 0X 
    END Prefixed;
static BOOLEAN OfrontOPC_Prefixed (OfrontOPT_ConstExt x, CHAR *y, LONGINT y__len)
{
    INTEGER i;
    __DUP(y, y__len, CHAR);
    i = 0;
    while ((*x)[__X(i + 1, 256)] == y[__X(i, y__len)]) {
        i += 1;
    }
    __DEL(y);
    return y[__X(i, y__len)] == 0x00;
}

Look at the last line of the C function. Here is the access into the memory which is already disposed. The way of alloca meant empty definition of DEL(), and everything worked properly. But now DEL() is

#define __DEL(x) Platform_OSFree((LONGINT)(uintptr_t)x)

So we have to solve this problem somehow. Such a solution, as below, as you have now, I find unsatisfactory.

    PROCEDURE Prefixed(x: OPT.ConstExt;  y: ARRAY OF CHAR): BOOLEAN;
        VAR i: INTEGER;  r: BOOLEAN;
    BEGIN i := 0;
        WHILE x[i+1] = y[i] DO INC(i) END ;
        r := y[i] = 0X;
        RETURN r;
    END Prefixed;

If you are interested in how I'm doing. Now I will do:

    PROCEDURE Prefixed(x: OPT.ConstExt; y: ARRAY [1] OF CHAR): BOOLEAN;
        VAR i: INTEGER;
    BEGIN i := 0;
        WHILE x[i+1] = y[i] DO INC(i) END ;
        RETURN y[i] = 0X
    END Prefixed;

And probably I'll go back to the use of alloca. I do not like this, but I did not come up with a better solution.

dcwbrown commented 7 years ago

I believe this is fundamentally a problem that should be fixed in the compiler, so

in voc the compiler always generates a dedicated variable for the result and

evaluates the expression on the return statement into it before calling __DEL.

Arguably the compiler should be more intelligent and avoid generating a dedicated

result variable if the return statement does not reference the deleted parameter copy,

but that's quite a big compiler change, so I excused myself by arguing that

the C compiler ought to optimise it to the same code anyway.

The change is here:

https://github.com/dcwbrown/olang.old/commit/e3ca6f6961c4dfbc72f469114f02601cd99c3b0a

-- Dave.

On 2016-08-01 13:33, Oleg N. Cher wrote:

Dear David.

Once you've got rid of alloca in favor of an explicit allocation and deallocation, there was a problem that still doesn't solved, only masked.

PROCEDURE Prefixed(x: OPT.ConstExt; y: ARRAY OF CHAR): BOOLEAN; VAR i: INTEGER; BEGIN i := 0; WHILE x[i+1] = y[i] DO INC(i) END ; RETURN y[i] = 0X END Prefixed;

static BOOLEAN OfrontOPC_Prefixed (OfrontOPT_ConstExt x, CHAR _y, LONGINT ylen) { INTEGER i; DUP(y, ylen, CHAR); i = 0; while ((_x)[X(i + 1, 256)] == y[X(i, y__len)]) { Console_Char(y[X(i, ylen)]); i += 1; } if (y[X(i, y__len)] == 0x00) { ConsoleString((CHAR)" YES! ", (LONGINT)7); } else { ConsoleString((CHAR)" NO! ", (LONGINT)6); } Console_Ln(); DEL(y); return y[X(i, y__len)] == 0x00; }

Look at the last line of the C function. Here is the access into the memory which is already disposed. The way of alloca meant empty definition of DEL(), and everything worked properly. But now DEL() is

define __DEL(x) Platform_OSFree((LONGINT)(uintptr_t)x)

So we have to solve this problem somehow. Such a solution, as below, as you have now, I find unsatisfactory.

PROCEDURE Prefixed(x: OPT.ConstExt; y: ARRAY OF CHAR): BOOLEAN; VAR i: INTEGER; r: BOOLEAN; BEGIN i := 0; WHILE x[i+1] = y[i] DO INC(i) END ; r := y[i] = 0X; RETURN r; END Prefixed;

If you are interested in how I'm doing. Now I will do:

PROCEDURE Prefixed(x: OPT.ConstExt; y: ARRAY [1] OF CHAR): BOOLEAN; VAR i: INTEGER; BEGIN i := 0; WHILE x[i+1] = y[i] DO INC(i) END ; RETURN y[i] = 0X END Prefixed;

And probably I'll go back to the use of alloca. I do not like this, but I did not come up with a better solution.

You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub [1], or mute the thread [2].

Links:

[1] https://github.com/vishaps/voc/issues/39 [2] https://github.com/notifications/unsubscribe-auth/ADChoEZa19J4SQULWKmjza9-QkPh46zaks5qbeehgaJpZM4JZiT_

dcwbrown commented 7 years ago

Oh, by the way, the commit I referenced below also includes a change to OPC.Mod

procedure Prefixed which predates the real fix and is not required.

-- Dave.

On 2016-08-01 14:19, dave wrote:

I believe this is fundamentally a problem that should be fixed in the compiler, so

in voc the compiler always generates a dedicated variable for the result and

evaluates the expression on the return statement into it before calling __DEL.

Arguably the compiler should be more intelligent and avoid generating a dedicated

result variable if the return statement does not reference the deleted parameter copy,

but that's quite a big compiler change, so I excused myself by arguing that

the C compiler ought to optimise it to the same code anyway.

The change is here:

https://github.com/dcwbrown/olang.old/commit/e3ca6f6961c4dfbc72f469114f02601cd99c3b0a

-- Dave.

On 2016-08-01 13:33, Oleg N. Cher wrote:

Dear David.

Once you've got rid of alloca in favor of an explicit allocation and deallocation, there was a problem that still doesn't solved, only masked.

PROCEDURE Prefixed(x: OPT.ConstExt; y: ARRAY OF CHAR): BOOLEAN; VAR i: INTEGER; BEGIN i := 0; WHILE x[i+1] = y[i] DO INC(i) END ; RETURN y[i] = 0X END Prefixed;

static BOOLEAN OfrontOPC_Prefixed (OfrontOPT_ConstExt x, CHAR _y, LONGINT ylen) { INTEGER i; DUP(y, ylen, CHAR); i = 0; while ((_x)[X(i + 1, 256)] == y[X(i, y__len)]) { Console_Char(y[X(i, ylen)]); i += 1; } if (y[X(i, y__len)] == 0x00) { ConsoleString((CHAR)" YES! ", (LONGINT)7); } else { ConsoleString((CHAR)" NO! ", (LONGINT)6); } Console_Ln(); DEL(y); return y[X(i, y__len)] == 0x00; }

Look at the last line of the C function. Here is the access into the memory which is already disposed. The way of alloca meant empty definition of DEL(), and everything worked properly. But now DEL() is

define __DEL(x) Platform_OSFree((LONGINT)(uintptr_t)x)

So we have to solve this problem somehow. Such a solution, as below, as you have now, I find unsatisfactory.

PROCEDURE Prefixed(x: OPT.ConstExt; y: ARRAY OF CHAR): BOOLEAN; VAR i: INTEGER; r: BOOLEAN; BEGIN i := 0; WHILE x[i+1] = y[i] DO INC(i) END ; r := y[i] = 0X; RETURN r; END Prefixed;

If you are interested in how I'm doing. Now I will do:

PROCEDURE Prefixed(x: OPT.ConstExt; y: ARRAY [1] OF CHAR): BOOLEAN; VAR i: INTEGER; BEGIN i := 0; WHILE x[i+1] = y[i] DO INC(i) END ; RETURN y[i] = 0X END Prefixed;

And probably I'll go back to the use of alloca. I do not like this, but I did not come up with a better solution.

You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub [1], or mute the thread [2].

Links:

[1] https://github.com/vishaps/voc/issues/39 [2] https://github.com/notifications/unsubscribe-auth/ADChoEZa19J4SQULWKmjza9-QkPh46zaks5qbeehgaJpZM4JZiT_

Oleg-N-Cher commented 7 years ago

Thank you for the answer, David.

Then you can rollback the procedure Prefixed to the previous state.

And how about the presence RETURN not only at the end, and in the body of a procedure? Like

    PROCEDURE Prefixed(x: OPT.ConstExt; y: ARRAY [1] OF CHAR): BOOLEAN;
        VAR i: INTEGER;
    BEGIN i := 0; IF i # 0 THEN RETURN y[i] = 0X END;
        WHILE x[i+1] = y[i] DO INC(i);
        IF i # 0 THEN RETURN y[i] = 0X END;
        END ;
        RETURN y[i] = 0X
    END Prefixed;

Everything is work properly too?

I'm sorry, but I have not tested on voc since I do not have a binary distribution of it.

dcwbrown commented 7 years ago

Yes, it handles return correctly wherever you put it.

By the way you don't need a binary distribution of voc to build it - it comes with pre-prepared bootstrap

C source that is ready to go. It's all handled automatically for you.

Just git clone https://github.com/vishaps/voc, cd into voc and run make full.

-- Dave.

On 2016-08-01 16:05, Oleg N. Cher wrote:

Thank you for the answer, David.

Then you can rollback the procedure Prefixed to the previous state.

And how about the presence RETURN not only at the end, and in the body of a procedure? Like

PROCEDURE Prefixed(x: OPT.ConstExt; y: ARRAY [1] OF CHAR): BOOLEAN; VAR i: INTEGER; BEGIN i := 0; IF i # 0 THEN RETURN y[i] = 0X END; WHILE x[i+1] = y[i] DO INC(i); IF i # 0 THEN RETURN y[i] = 0X END; END ; RETURN y[i] = 0X END Prefixed;

Everything is work properly too?

I'm sorry, but I have not tested on voc since I do not have a binary distribution of it.

You are receiving this because you commented. Reply to this email directly, view it on GitHub [1], or mute the thread [2].

Links:

[1] https://github.com/vishaps/voc/issues/39#issuecomment-236608155 [2] https://github.com/notifications/unsubscribe-auth/ADChoDcpCN09QpPVOgGI40etRBHvsRq7ks5qbgtVgaJpZM4JZiT_