llvm dev - Nov 2005 - [LLVMdev] llvm-ranlib: Bus Error in regressions + fix

Evan Jones wrote:
> On Nov 23, 2005, at 12:08, Reid Spencer wrote:
> 
>>> However, it would be possible to use a stringstream for this.
>>
>> Aggg! No! Those perform horribly with anything more than a small 
>> amount of data. Some Archive files can be huge (e.g. not fit in
memory).
> 
> 
> An archive can be too big to fit in memory? That would be a BIG library. 
Yes, but they do exist. I've seen 1GByte archive files.
> In that case, building a temporary in memory is a bad idea, but then so 
> is copying it twice, as is being done now. Maybe it really would be 
> better to scan over everything and build the symbol table, and then scan 
> over everything again and build the final archive. This would avoid 
> writing an extra copy out to disk, and it wouldn't be hard to do this.
Yup, that would work too. It would, however, require significant rework in 
ArchiveWriter.cpp code.
>> Yup. If you use the sys::Path class to do the "unlink" 
>> (removeFromDisk) then the platform differences should be accounted 
>> for. Please don't put the direct unlink call into
ArchiveWriter.cpp.
> 
> So this comment means that I should attempt to theoretically support 
> Windows, and close the current archive file before updating it?
If you use sys::Path, it is not "theoretically" supported, it is
supported.
To remove a file, use sys::Path::removeFromDisk, not unlink(2).

  > I'll remove the stringstream, but this means that I need to create
two
> temporary files. However, this change is less invasive, so I'll rework 
> my patch again.
Sounds good. We can change to the two-pass reading (when building a symbol 
table) some other time. I'm sure you just want to get this little issue done
and over with :)

Reid.

On Nov 23, 2005, at 18:17, Reid Spencer wrote:
>> So this comment means that I should attempt to theoretically support 
>> Windows, and close the current archive file before updating it?
> If you use sys::Path, it is not "theoretically" supported, it is 
> supported.
Well, except of course that it isn't currently possible to build LLVM 
on Windows. Hence this is a somewhat theoretical issue.
> Sounds good. We can change to the two-pass reading (when building a 
> symbol table) some other time. I'm sure you just want to get this 
> little issue done and over with :)
I have a problem sometimes: I need to find *why* something goes wrong.

At any rate, I've attached the latest version of my patch. It has some 
interesting details:

1. Before replacing the original archive file, it invalidates all the 
members of the current Archive instance. This required me to refactor 
the destructor code into a separate function. This should make this 
code work on Windows.

2. I fixed a memory leak in the Archive destructor. The "foreignST" 
member was never deleted.

3. There are now two temporary files created if we need to build the 
symbol table, and they are moved over top of the original archive once 
the process is complete.

Let me know if there are any comments. The part where the second 
temporary file is kind of gross, since it needs to catch exceptions and 
be erased if it crashes.

Evan Jones

-------------- next part --------------
A non-text attachment was scrubbed...
Name: archive.patch
Type: application/octet-stream
Size: 5977 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20051123/71374df4/attachment.obj>
-------------- next part --------------

--
Evan Jones
http://evanjones.ca/

Evan Jones wrote:
> On Nov 23, 2005, at 18:17, Reid Spencer wrote:
>
>>> So this comment means that I should attempt to theoretically
support
>>> Windows, and close the current archive file before updating it?
>>
>> If you use sys::Path, it is not "theoretically" supported, it
is
>> supported.
>
>
> Well, except of course that it isn't currently possible to build LLVM 
> on Windows. Hence this is a somewhat theoretical issue.
>
News to me.  I do it all the time.  Notice the "win32" directory under
the root :)

Evan,

Your patch passed tests on my system and is now committed. Thanks!

Reid.

Evan Jones wrote:
> On Nov 23, 2005, at 18:17, Reid Spencer wrote:
> 
>>> So this comment means that I should attempt to theoretically
support
>>> Windows, and close the current archive file before updating it?
>>
>> If you use sys::Path, it is not "theoretically" supported, it
is
>> supported.
> 
> 
> Well, except of course that it isn't currently possible to build LLVM
on
> Windows. Hence this is a somewhat theoretical issue.
> 
>> Sounds good. We can change to the two-pass reading (when building a 
>> symbol table) some other time. I'm sure you just want to get this 
>> little issue done and over with :)
> 
> 
> I have a problem sometimes: I need to find *why* something goes wrong.
> 
> At any rate, I've attached the latest version of my patch. It has some 
> interesting details:
> 
> 1. Before replacing the original archive file, it invalidates all the 
> members of the current Archive instance. This required me to refactor 
> the destructor code into a separate function. This should make this code 
> work on Windows.
> 
> 2. I fixed a memory leak in the Archive destructor. The
"foreignST"
> member was never deleted.
> 
> 3. There are now two temporary files created if we need to build the 
> symbol table, and they are moved over top of the original archive once 
> the process is complete.
> 
> Let me know if there are any comments. The part where the second 
> temporary file is kind of gross, since it needs to catch exceptions and 
> be erased if it crashes.
> 
> Evan Jones
> 
> 
> -- 
> Evan Jones
> http://evanjones.ca/
> 
> 
> ------------------------------------------------------------------------
> 
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev