Code audit

[pip88nl]

Hello Aldor List,

while reading, modifying and testing bits and pieces of the compiler,
I came across many potentially unsafe fragments. These fragments
include unsigned/signed comparisons, truncating assignments, unchecked
integer overflow, etc. Things like that. Generally, these should not
be problematic, but using an int as loop variable where the loop
condition compares with Length is both non-descriptive and wrong on
certain platforms. Where on 32 bit architectures, int and Length are
probably equally sized, on 64 bit machines that use the LP64 data
model (for instance gcc on linux/x86_64), Length is larger than int
and therefore, the loop variable may not be able to loop through each
element, causing infinite loops.

I suggest a thorough audit of the code, pointing out each arguable
comparison, assignment, etc. There are several occasions where the
modification required is straightforward, but many are not as clear
and require deep knowledge of the code. Deep, by the way, is not just
knowing what the code does, but more importantly, what the code was
supposed to do when it was written. Who is up for some more active
development?

Regards,

--
Pippijn van Steenhoven

_______________________________________________
Aldor-l mailing list
Aldor-l@...
http://aldor.org/mailman/listinfo/aldor-l_aldor.org

[Ralf Hemmecke]

Dear Pippijn,

I cannot say that I will be of much help since I don't have any
knowledge of the compiler internals. And I am not fluent in LISP and C.
Nevertheless, I'd be interested in looking more closely at the code so
that eventually nasty bugs will be removed and a more stable compiler
evolved.

Take the lead and just start discussing particular code chunks.
Hopefully that at least results in a more documented code base.

Ralf

On 06/24/2008 10:32 PM, Pippijn van Steenhoven wrote:

> Hello Aldor List,
>
> while reading, modifying and testing bits and pieces of the compiler,
> I came across many potentially unsafe fragments. These fragments
> include unsigned/signed comparisons, truncating assignments, unchecked
> integer overflow, etc. Things like that. Generally, these should not
> be problematic, but using an int as loop variable where the loop
> condition compares with Length is both non-descriptive and wrong on
> certain platforms. Where on 32 bit architectures, int and Length are
> probably equally sized, on 64 bit machines that use the LP64 data
> model (for instance gcc on linux/x86_64), Length is larger than int
> and therefore, the loop variable may not be able to loop through each
> element, causing infinite loops.
>
> I suggest a thorough audit of the code, pointing out each arguable
> comparison, assignment, etc. There are several occasions where the
> modification required is straightforward, but many are not as clear
> and require deep knowledge of the code. Deep, by the way, is not just
> knowing what the code does, but more importantly, what the code was
> supposed to do when it was written. Who is up for some more active
> development?

_______________________________________________
Aldor-l mailing list
Aldor-l@...
http://aldor.org/mailman/listinfo/aldor-l_aldor.org

[pip88nl]

Hi all,

in foam.c: foamFrString (String), a Buffer is created, used and then
liberated. It is not freed. Why not?

Code:

Foam
foamFrString(String s)
{
        Foam    foam;
        Buffer  buf;

        buf = bufCapture(s, stoSize(s));
        foam = foamFrBuffer(buf);
        bufLiberate(buf); // <- here

        return foam;
}

--
Pippijn van Steenhoven


_______________________________________________
Aldor-l mailing list
Aldor-l@...
http://aldor.org/mailman/listinfo/aldor-l_aldor.org

[pip88nl]

Hi, here are some more areas of doubt:

genfoam.c:7335 (genGetConstNums)
  genHasConstNum is a function which (as its name tells us) checks
  something and returns true or false. The return value of the function
  is not being used for anything, which makes me wonder whether this code
  is correct. Can someone please tell me more about this?

gf_add.c:2152 (gen0RtSefoHashId)
  Same thing as above, gen0Syme generates something but the value is not
  used.

gf_seq.c:1129 (dgOrderError)
  bufLiberate is called with the comment /* Free the buffer and other
  storage */. While this is true, the memory for the internal string is
  not freed.

gf_seq:1069 (dgCycleError)
  Same as dgOrderError.

inlutil.c:309 (inuProgFini)
  flogToProg reconstructs a foam program from a flow graph. The Foam
  object returned is not used. I believe it also forces an update to
  flog->prog, so it might be sane.

of_jflow.c:159 (jflowProg)
  Same as above.

optfoam.c:426 (optimizeFoam)
  foamAuditAll is called. This does some checking on e.g. casts, types,
  etc. It returns a boolean result, which is not used. Does it make sense
  to call the audit in that case?

util.c:389 (DFloatSprint)
  Returns the character buffer it received. Would it not be better to
  return the return value from the sprintf called inside the function?
  Returning the string gives us no information at all.

stab.c:1351 (stabDefLibrary)
  tfFrSyme is called, the return value is not used.

stab.c:1372 (stabDefArchive)
  Same as above.


Some of these are more serious than others. Some may not be harmful at
all, but if someone knows the code, I would like to know what to think of
them.

--
Pippijn van Steenhoven


_______________________________________________
Aldor-l mailing list
Aldor-l@...
http://aldor.org/mailman/listinfo/aldor-l_aldor.org

[David Casperson]

Pippijn, you wrote:

> Hi all,
>
> in foam.c: foamFrString (String), a Buffer is created, used and then
> liberated. It is not freed. Why not?
>
> Code:
>
> Foam
> foamFrString(String s)
> {
>         Foam    foam;
>         Buffer  buf;
>
>         buf = bufCapture(s, stoSize(s));
>         foam = foamFrBuffer(buf);
>         bufLiberate(buf); // <- here
>
>         return foam;
> }

I am also learning compiler internals by reading code.  My
understanding is that the difference between bufFree and
bufLiberate is that the latter does not free the memory that the
buffer points at, just the buffer itself.  This matches
bufCapture, which does not copy the ``String s'', just makes a
buffer that points at it.

Does this seem right?


David
--
Dr. David Casperson, Assistant Professor     |  casper@...
Department of Computer Science               |  (250)   960-6672 Fax [phone]
College of Science and Management            |  3333 University Way
University of Northern British Columbia      |  Prince George, BC   V2N 4Z9
                                              |  CANADA


_______________________________________________
Aldor-l mailing list
Aldor-l@...
http://aldor.org/mailman/listinfo/aldor-l_aldor.org

[pip88nl]

On Mon, Jun 30, 2008 at 10:47:51AM -0700, David Casperson wrote:
> buffer points at, just the buffer itself.  This matches
> bufCapture, which does not copy the ``String s'', just makes a
> buffer that points at it.
>
> Does this seem right?

Yes, that seems right. I didn't remember that.

--
Pippijn van Steenhoven


_______________________________________________
Aldor-l mailing list
Aldor-l@...
http://aldor.org/mailman/listinfo/aldor-l_aldor.org

[pip88nl]

On Mon, Jun 30, 2008 at 10:47:51AM -0700, David Casperson wrote:
> buffer points at, just the buffer itself.  This matches
> bufCapture, which does not copy the ``String s'', just makes a
> buffer that points at it.
>
> Does this seem right?

It seems right, but in some cases (dgCycleError, dgOrderError), the
buffer is created with bufNew, not bufCapture. Your explanation of
"memory leak is okay on error paths" is not really what I'm looking for.
As I explained on my website, the Aldor GC performs best if memory is
manually returned to the allocator. Am I missing anything here? Is
bufLiberate really the right thing even in these functions?

--
Pippijn van Steenhoven


_______________________________________________
Aldor-l mailing list
Aldor-l@...
http://aldor.org/mailman/listinfo/aldor-l_aldor.org

[David Casperson]

Pippijn, you wrote:

> On Mon, Jun 30, 2008 at 10:47:51AM -0700, David Casperson wrote:
> > buffer points at, just the buffer itself.  This matches
> > bufCapture, which does not copy the ``String s'', just makes a
> > buffer that points at it.
> >
> > Does this seem right?
>
> It seems right, but in some cases (dgCycleError, dgOrderError), the
> buffer is created with bufNew, not bufCapture. Your explanation of
> "memory leak is okay on error paths" is not really what I'm looking for.
> As I explained on my website, the Aldor GC performs best if memory is
> manually returned to the allocator. Am I missing anything here? Is
> bufLiberate really the right thing even in these functions?

The code in dgCycleError and dgOrderError looks erroneous to me,
as it appears to not free the string contents of the buffer.

Stephen, Laurentiu ?


David
--
Dr. David Casperson, Assistant Professor     |  casper@...
Department of Computer Science               |  (250)   960-6672 Fax [phone]
College of Science and Management            |  3333 University Way
University of Northern British Columbia      |  Prince George, BC   V2N 4Z9
                                              |  CANADA


_______________________________________________
Aldor-l mailing list
Aldor-l@...
http://aldor.org/mailman/listinfo/aldor-l_aldor.org