vishapoberon / compiler

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

32bit builds fail #67

Open norayr opened 6 years ago

norayr commented 6 years ago

I see Dave fixed the png screen which displays the build status, and 32bit builds fail on Files.Mod after recent changes.

norayr commented 6 years ago

rolled back Files.Mod related changes.

dcwbrown commented 6 years ago

I'm sorry it's taking me a long time to get to this. I am not going to make any changes until I have a full set of test builds running again.

My goal to set up a freenas machine as a server of virtual machines, but it isn't going as smoothly as I hoped: the bhyve VM manager provides a console display only for UEFI boots, but half of the vms I want to set up are 32 bit and have no UEFI support. Which makes life difficult.

It's important to me to get this working and I'll get there one way or another.

I would like to see the test suite updated with a long file name test. I would like to understand exactly where the wrong file name variable is being overwritten - independent of any extra special-case tests added in files.mod this just shouln't happen. I can think of two reasons - either it's in Oberon code and not detected because we've turned off tests for the compiler build. In which case (actually in any case) we should be building the compiler with tests on. Or it's in the OS call, in which case there should be code added there to protect the Oberon code from this calls C and the OS. Or it may be something else.

dcwbrown commented 6 years ago

I have enabled index tests in OPT.Mod and Files.Mod. This leaves only Heap.Mod with tests disabled. I was pleased to find all builds and tests succeeded.

With these tests enabled the deep directory path no longer causes a memory overwrite: instead 'index out of range' is correctly reported.

I have also implemented explicit tests in Files.Mod for filenames that are too long for the Files.Mod implementation - such cases HALT with a 'Filename too long' error.

I considered exposing platform file name and path length limits in SYSTEM following runkharr's example, but I preferred to expose them less globally - from Files.Mod. They are now available as Files.MaxPathLength and Files.MaxNameLength.

For many Linux systems, MaxNameLength is 256, and MaxPathLength is 4096. Because these figures are constants from limits.h they cannot take into account the limits of specific installed filesystems. They shoud probably be taken with a grain of salt.

Memory usage: Files.Mod stores two name strings for each file it is accessing (e.g. the applications desired name and a temporrary file name.) Vishap Oberon had been allocating 100 characters for each, so a Files.Mod File object took a little over 200 bytes - which is perhaps an acceptable memory cost for an object which may be instatntiated many times. Supporting really deep filesystems is less happy though: Using MaxPathLength we'd be allocating two by 4096 bytes for each File object. In the vast majority of cases only a tiny fraction of those 8kb would actually be used.

So for maximum generality the fix should be considerably more invasive - the filenames should not be arrays of fixed size in the file decriptor, but rather pointers to filenames whose length is allocated according to the actual names.

This is a much bigger change. So the question is: Is it appropiate and consistent with the Oberon system?

Arguably not: Files.Mod is an interface that provides file access in the style of the Oberon system. File lifetime and buffering are managed quite differently from Posix style systems. There are no directory APIs, Oberon presents a flat filesystem. (Indeed the original Oberon system had a fixed filename length of 32 characters.)

How much do we want to support customers acessing files the Posix way?

I would argue that properly supporting Posix is not possible in Files.Mod. We would need a new module written rather differently from Files.Mod, less sophisticated by file lifetimes and buffering, and providing more APIs for directory management. It would be a thin layer over the functionality currently implemented in Platformunix.Mod and Platformwindows.Mod.

Actually, users can IMPORT Platform.Mod and use the low level APIs themselves.

So, has enough been done now?

There are many existing OS tools written in C for Windows and Linux that are limited to 256 characters. I feel that extending Files.Mod to arbitrary filename lengths is not a priority, attractive though it may be.

So I propose stopping here - we now support filenames including path up to 256 characters, with minimal changes to the Oberon code. We have fixed memory overwrites from long filenames, and we diagnose long filenames constructed by the Files.Mod code.

dcwbrown commented 6 years ago

Hmm, still thinking: I'm seriously doubting the wisdom of exposing MaxNameLength and MaxPathLength in Files.Mod.

The more I Iook at it the more I believe the exposed values are too ambiguous. They do not deserve the respectability of being exposed in Files.Mod.

Exposing them from Platform.Mod is more reasoanble. This reflects that their caveats are the direct repsonsibilty of the platfrom that Oberon is running on.

I've also seen a potential flaw in the temporary file handling code. Consider this sequence:

Register will fail, because it will attempt to rename the temporary file created on the first device/partition to it's registered name on the new device/partition.

Now GetTempName, the procedure used to create the name of the temporary backing file, goes to the trouble of including the current directory in the generated temporary file name.

This has the advantage that the temporary file name is stored independent of the current directory, and so won't become inaccessible to the rename by ChangeDirectory requests.

This is the one place in the Files.Mod code where a full absolute file name is required, but this cost only delivers half a fix for the issues caused by calling ChangeDirectory between New and Register. It doesn't solve ChangeDirectory changing to a new device/partition.

Global state such, as a 'current directory' setting, is a commonly discouraged idea. Indeed the official Oakwood spec has no ChangeDirectory. Vishap's Files.ChangeDirectory is not standard. I wonder when it was added?

I think we're free to fix this by defining that a relative filename passed to Files.New is relative to the current directory at the time of the Files.New call. (In the current code it is relative to the current directory when the file is registered.)

The solution I see is to record the current directory in the Files.File at Files.New time, and use it both at temporary file creation time and at register time. This would have a couple of benefits:

Make sense?

dcwbrown commented 6 years ago

I've added doc/Files.md describing differences between how most operating systems treat files and filesystems, and how Oberon Files.Mod does it.

Based on this understanding, and the issues with filename lengths above, I'm considering having a go at simplifying Files.Mod a little and making it independent of filename length at the same time.