vermaseren / form

The FORM project for symbolic manipulation of very big expressions
GNU General Public License v3.0
985 stars 118 forks source link

Trying to open a nonexisting table doesn't generate an error #542

Open vsht opened 1 week ago

vsht commented 1 week ago

Hi,

I'm not sure if it's an omission or was originally intended like that, but I noticed that trying to open a tablebase that physically doesn't exist on disk doesn't generate any errors.

TableBase "blalba.tbl" open;
TableBase "blalba.tbl" enter;
L exp = 42;

.end

Only when trying to insert an element from a nonexisting table will this code crash as it should.

Cheers, Vlad

jodavies commented 1 week ago

For me FORM does crash here, though it should certainly give a useful error message.

    Tablebase "test.tbl" open;
    Tablebase "test.tbl" enter;
Program terminating at open.frm Line 8 -->

It's a segfault since "enter" tries to loop through the tables without checking if there are any.

Note that the open does actually create a file (since if it doesn't find the file, it just calls NewDbase as "create" does). The following works just fine:

CTable,sparse f(1);
Fill f(1) = 1;
Fill f(2) = 2;
.sort

Tablebase "test.tbl" open;
Tablebase "test.tbl" addto f;
.end
vsht commented 1 week ago

For me FORM does crash here, though it should certainly give a useful error message.

Ok, may be my version is too old: FORM 4.1 (May 15 2023, v4.1-20131025-587-g8a37a42).

But I agree that a useful error message would be nice.

Note that the open does actually create a file (since if it doesn't find the file, it just calls NewDbase as "create" does).

Is this expected? According to the manual

This opens an existing file with the indicated name. It is assumed that the file has been created with the ‘create’ option in a previous FORM program. It gives the user access to the contents of the tablebase. In addition it allows the user to add to its contents.

This doesn't read like a new file will be created if it doesn't exist so far.

jodavies commented 1 week ago

The behaviour regarding the tablebases should not have changed here since 8a37a42 . Can you try running the debug binary on your script under gdb?

vsht commented 1 week ago

Ok, now I see the problem. When I run this code for the first time, it will crash but at the same time create an empty blabla.tbl. Then, running it for the second time will not generate any crashes.

Tbh, I find this behavior quite confusing, since in real life the table might be missing for a reason and substituting it with an empty one is definitely not what the user would like.

jodavies commented 1 week ago

It would be straightforward to have "open" give an error and terminate FORM, in case the file doesn't exist. This could break people's existing FORM scripts in principle, so let's see if anyone else has opinions on this change.

It is also not so nice that "create" overwrites existing files with the same name. Here one could check for an existing file and terminate if one is found.

vsht commented 1 week ago

Is there another way to check for the physical existence of the table before "opening" it?

audit seemingly does what I want, but I also don't want to pollute the logs by printing the full table content.

jodavies commented 1 week ago

I don't think so with the tablebase command. Of course you can use #system or #pipe to make such a check.

vsht commented 1 week ago

I don't think so with the tablebase command. Of course you can use #system or #pipe to make such a check.

Thanks. I also thought about that, but of course a native FORM command would be nicer.

tueda commented 1 week ago

but of course a native FORM command would be nicer.

So far, no one has tried to implement it: #240

vsht commented 1 week ago

Speaking of pipes, I noticed that trying to check for the existance of a table using #pipe and #define also leads to a somewhat strange behavior

#pipe echo "#define TABLEPRESENT \"$((ls blalba.tbl >> /dev/null 2>&1 && echo yes) || echo no)\""

#message `TABLEPRESENT'

#if (`TABLEPRESENT'!="yes")        
  exit "Error, the table is missing.";
#endif

TableBase "blalba.tbl" open;
TableBase "blalba.tbl" enter;
L exp = 42;

.end

My expectation is that if TABLEPRESENT not equals yes, the code should stop immediately due to the exit statement. However, in reality the execution continues after exit and a fake blalba.tbl still gets generated :(

It works if I use something like

#if (`TABLEPRESENT'!="yes")        
  exit "Error, the table is missing.";
.end
#endif

but this really contradicts the main idea of using exit in the first place.

vsht commented 1 week ago

It would be straightforward to have "open" give an error and terminate FORM, in case the file doesn't exist. This could break people's existing FORM scripts in principle, so let's see if anyone else has opinions on this change.

It is also not so nice that "create" overwrites existing files with the same name. Here one could check for an existing file and terminate if one is found.

A possible compromise would be to change the manual stating that open will also create an empty table if it doesn't exist so far. Then adding something like shortaudit as a way to check for the existence of the table without polluting the logs would be more than sufficient. The current behavior is IMHO not really ok.

jodavies commented 1 week ago

You need to use a preprocessor terminate:

#if (`TABLEPRESENT'!="yes")
   #message "Error, the table is missing."
   #terminate
#endif

Exit is an executable statement and you are not processing any terms when you make the pre-processor check.

My preference would be to make open fail if the file doesn't exist, rather than add shortaudit or anything.

vsht commented 1 week ago

Exit is an executable statement and you are not processing any terms when you make the pre-processor check.

Ok, that makes sense. Thanks.

My preference would be to make open fail if the file doesn't exist, rather than add shortaudit or anything.

That would be an ideal solution, but at the cost of breaking someone's scripts who was silently using this bug as a feature.

jodavies commented 1 week ago

There should also be #531 merged at some point, readonly mode should certainly fail if the file doesn't exist.