magic icon indicating copy to clipboard operation
magic copied to clipboard

Fix infinite def write

Open kareefardi opened this issue 1 year ago • 5 comments

I think .def_part is resolved to .def resulting in infinite write behavior. This renames the partial file .._part.def avoiding the infinite loop while writing def.

I am not sure however if freeMagic(outName_part) is called in all the correct places.

kareefardi avatar Jul 30 '24 09:07 kareefardi

@dlmiles : My problem with the original pull request is that I still don't understand the nature of the infinite loop and I don't see how the code change fixes it, and I don't understand the need to change the file name being written out. This is mostly do to me having limited time to check and test code changes.

RTimothyEdwards avatar Oct 09 '24 13:10 RTimothyEdwards

Understand, just trying to help out with the code review aspect to point out strlen(outName) is problematic because outName can be NULL. The returned variable filename looks a better variable to use.

I am not affected by the problem this commit is trying to address.

@kareefardi is the infinite loop an internal magic infinite loop? Or an issue with external tooling seeing or not seeing well formed files with the appropriate file extension?

Is there an example command sequence to run with project files to demonstrate the original problem?

dlmiles avatar Oct 09 '24 14:10 dlmiles

@dlmiles @RTimothyEdwards I was able to isolate the problem to it being caused by calling def write spm.def instead of def write spm. Here is a demonstration of this: demo

kareefardi avatar Oct 14 '24 09:10 kareefardi

diff --git a/lef/lefWrite.c b/lef/lefWrite.c
index 024fdf2a..639cef78 100644
--- a/lef/lefWrite.c
+++ b/lef/lefWrite.c
@@ -191,10 +191,12 @@ lefFileOpen(def, file, suffix, mode, prealfile)
     {
        if (strcmp(endp, suffix))
        {
+#if 0
            /* Try once as-is, with the given extension.  That takes care   */
            /* of some less-usual extensions like ".tlef".                  */
            if ((rfile = PaOpen(name, mode, NULL, Path, CellLibPath, prealfile)) != NULL)
                return rfile;
+#endif
 
            len = endp - name;
            if (len > sizeof namebuf - 1) len = sizeof namebuf - 1;

This tries a re-open of the given filename "spm.def" and this is a hit and is successful, so it returns a handle as-is. This is due to the suffix=".def_part" not matching the "spm.def" extension.

When the given filename is "spm" I guess it can't see an extension to try to match, so this is doesn't try this "optimization"

dlmiles avatar Oct 14 '24 12:10 dlmiles

Order of events:

DefWriteCell(outName="spm.def") // or outName="spm" which does not trigger issue
f = lefFileOpen(def, outName, ".def", "w", &filename);
## 'f' has data written into file

f2 = lefFileOpen(def, outName, ".def_part", "w", &filename);
## data is written into 'f2'  this is acts like a tmpfile I guess due to out-of-order data processing
## but it is in the same directory as the outName
## it is unclear (and maybe not important) if this file actually unique and gets created, it maybe truncating 'f' but both files have data written into them now

## f2 has data written into it
fclose(f2);

## f1 has more data written into it (call it 512M of data)

f2 = lefFileOpen(def, outName, ".def_part", "r", &filename);
## f2 is reopening the previously processed data, but this reopen actually opens the same file as 'f' not a seperate

## at this point:
## 'f2' is RO handle on 'spm.def' at filepos=0
## 'f'is RW handle on 'spm.def' at filepos=512M
while (fgets(line, sizeof line, f2) != NULL) fprintf(f, "%s", line);
fclose(f2);
## this while loop causes infinite write, as the file handles are 512M apart of the same file

The optimization (if that is a good term) in the #if 0 commented out section, uses the outName as-is and has suffix=NULL in call to PaOpen() so it ignores the passed suffix=".def_part" passed to lefFileOpen() so when the optimization is triggered it opens spm.def

Previously (before this PR) outName is the same value to all calls to lefFileOpen(), which then derives the endp (extension) from that it can see in outName. endp=".def" suffix=".def_part"

By modifying outName to append _def_partfor the tempfile it permutes the filename enough so it can not match the 'f' filehandle filename of spm.def

dlmiles avatar Oct 18 '24 14:10 dlmiles

Fixed to my satisfaction in PR #359 , so I will now close this PR.

RTimothyEdwards avatar Jan 04 '25 16:01 RTimothyEdwards