[llvm-commits] [llvm] r47621 - in /llvm/trunk: include/llvm-c/lto.h tools/lto2/ tools/lto2/LTOCodeGenerator.cpp tools/lto2/LTOCodeGenerator.h tools/lto2/LTOModule.cpp tools/lto2/LTOModule.h tools/lto2/Makefile tools/lto2/lto.cpp tools/lto2/lto.exports

[Chris Lattner]

<catching up on old mail>

On Feb 27, 2008, at 2:22 PM, Nick Kledzik wrote:

>>> +#include <fstream>
>>> +#include <unistd.h>
>>> +#include <stdlib.h>
>>> +#include <fcntl.h>
>>
>> For standard c++ headers like stdlib, please use <cstdlib>.  Is it  
>> possible to avoid unistd.h/fcntl.h with the llvm/System/Path.h or  
>> MemoryBuffer.h stuff?  MemoryBuffer.h is an efficient way to read  
>> an entire file in off the disk.
> The issue is that lto_codegen_compile() returns a malloc'ed block  
> which the client frees when done.  I don't see a way to use  
> MemoryBuffer that allows me to transfer ownership of the underlying  
> buffer away and then delete the buffer.  Also if looks like  
> MemoryBuffer::getFile() mmaps the file instead of using malloc() so,  
> the client then can't free() it.   What I've done is change the  
> commensts for to_codegen_compile() to say that the returned buffer  
> is owned by the lto_codegen_t object and is freed with it is  
> disposed.  Then I got rid of the malloc/open/read and used  
> MemoryBuffer::getFile().

Nice

>>
>>> +    bool                writeMergedModules(const char* path,  
>>> std::string& errMsg);
>>
>> This goes over 80 columns.
> Fixed (I just threw that one in to see if the rumors were true, and  
> the are ;-)

Hehe, so you're just toying with me huh? ;-)

>> Please use RAII and early exits to simplify the code.  A useful  
>> thing for this is llvm/ADT/OwningPtr.h.  You should be able to do  
>> something like this:
>>
>>  OwningPtr<MemoryBuffer> buffer = MemoryBuffer::getMemBuffer...
>>
> Hey!  OwningPtr<> is not in the Xcode project.  That is how I've  
> been search llvm sources.

Yeah, unfortunately, the xcode project doesn't get much love :(

> I reworked this to use OwningPtr<> and factored out the target  
> matching code, and yes it is all much smaller now.  Cool!

Great!

>>> Wow, that is a hack.  :)  We should really fix this at the LLVM IR  
>>> level, can you please file a radar that demonstrates how not  
>>> distinguishing between these two cases cause a problem?
> We already have <rdar://problem/5684011> that Dale wrote up.

Ok

>>> +        // Use mangler to add GlobalPrefix to names to match  
>>> linker names.
>>> +        Mangler mangler(*_module, _target->getTargetAsmInfo()- 
>>> >getGlobalPrefix());
>>
>> There is one other aspect of configuration for the mangler: On  
>> darwin at least you have to call manger.setUseQuotes(true);
>>
>> This tells it that it is ok to make some symbols use double quotes  
>> (e.g. those with a space in them like objc methods) instead of  
>> mangling the space.  I don't know if we can currently get this from  
>> TargetAsmInfo.h, but I think we should be able to.  Dale or Evan  
>> can help with this part.

> I don't think this is applicable at this level.  You need double  
> quotes around ObjC method names in the generated assembly source  
> file because the otherwise the assembler can't parse the label  
> name.  The linker and LTO use raw const char* strings which can have  
> embedded spaces.

Ok, but I'm concerned about the mangler mangling strings in other  
ways.  If you have a name that contains a space, for example, when  
"quotes" are supported, the mangler will leave the space but wrap the  
name with quotes.  When quotes are not available, the mangler will  
replace the space with something gross.  Basically not setting quotes  
will get you a different string than enabling quotes and then removing  
the quotes from the name.

>>> +// holds most recent error string
>>> +// *** not thread safe ***
>>> +static std::string sLastErrorString;
>>
>> This also has a static ctor, which we try to avoid when possible  
>> (yes, we don't avoid very well, but we still try :).  Is there  
>> another way to do this?
> Even without it, there is still a static ctor in lto.o.  Something  
> to do with ManglerLinkVar and IncludeFile().
> Given that there are a couple hundred ctors in the libLTO.dylib, I'm  
> not to worried about this one.

Ok.

Thanks Nick!

-Chris
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20080321/4f7cae07/attachment.html>