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

Evan Jones wrote:
> On Nov 23, 2005, at 8:16, Evan Jones wrote:
> 
>> (4) Write the foreignST into the TmpArchive file. Is there any reason 
>> that this isn't possible? Then the final archive would be created
in a
>> single pass, and it could just be moved into place.
> 
> 
> Ah. I see: It needs to be written in order to compute the offsets. 
Exactly.
> 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).
> 
> I think the best thing to do is to write the final archive to a 
> temporary file, and then move it into place. This has the advantage that
> if something goes wrong the original archive will not be corrupted. 
> Additionally, all the members of the current archive object instance 
> stay valid, since the original mmap is still valid.
Yeah, that sounds like the safest/sanest strategy.
> 
> One disadvantage of this approach is that if CreateSymbolTable is false, 
> it will be slightly more expensive as it will build the archive in 
> memory before writing it to disk, instead of just writing it out to disk 
> directly.
You can always use CreateSymbolTable as a flag to determine which mechanism to
use.
> 
>> Also, since writeToDisk() invalidates the mapped file that was 
>> originally used to create the archive, shouldn't this function also
>> unmap that file and erase all its members? This would prevent and 
>> further bugs like this from happening.
> 
> 
> This is not necessary using the replace method described above. However, 
> it *would* be necessary on Windows since you can't unlink/replace an 
> open file on that platform. To fix this, the original archive file must 
> be closed before performing the move.
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.
> 
> I've attached a patch that builds the temporary archive in a 
> stringstream, then writes the temporary file and moves it into place. It 
> fixes the bug on the old RedHat system.
Please test the performance of that on the large .a file in the regression 
tests. I have my doubts about the scalability and speed for stringstream. Why 
can't you just write it out directly to the temporary file?

Reid.

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. 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.
>> This is not necessary using the replace method described above. 
>> However, it *would* be necessary on Windows since you can't 
>> unlink/replace an open file on that platform. To fix this, the 
>> original archive file must be closed before performing the move.
> 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?
>> I've attached a patch that builds the temporary archive in a 
>> stringstream, then writes the temporary file and moves it into place. 
>> It fixes the bug on the old RedHat system.
> Please test the performance of that on the large .a file in the 
> regression tests. I have my doubts about the scalability and speed for 
> stringstream. Why can't you just write it out directly to the 
> temporary file?
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.

Evan Jones

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

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.