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

[Reid Spencer]

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.