[PATCH][llvm-c] expose sys::getProcessTriple

[Jim Grosbach]

Hi Filip,

This seems generally fine. Just a couple of nitpicks.

Let’s make it explicit that it’s the host process string being retrieved since this will be used mainly in the MCJIT context and that it’s a triple being retrieved. "LLVMGetProcessTriple()” seems right to me.

Less important, but I’d prefer having the size argument to the function say that’s what it is, so something like, “BufferSize,” perhaps?

> + * size_t targetSize = LLVMGetProcessTarget(0, 0);


Use an explicit NULL here for the pointer value.

> "If TargetBytes isn't enough then the Target string is filled only up to TargetBytes; that is this function will never overflow that bound.”


Is the string null terminated when there’s not enough room for the whole string? IMO it should be.

-Jim



On Jun 28, 2013, at 2:15 PM, Filip Pizlo <fpizlo at apple.com> wrote:

> Ping!
> 
> -F
> 
> 
> On Jun 27, 2013, at 8:05 PM, Filip Pizlo <fpizlo at apple.com> wrote:
> 
>> The fact that this isn't already exposed makes using some other C APIs, like the disassembler API, really awkward in a JIT context - you don't want to have to describe to LLVM what the target name is for the process you're in, when LLVM already has this information.
>> 
>> The challenge here is that sys::getProcessTriple() returns a std::string that it allocates itself.  So we can't just have a "LLVMGetProcessTarget()" that returns a char*, unless we then require the user to free() that char*.  I considered that but I feared that it would have been confusing to users: for example LLVMGetTarget() returns a char* that doesn't require deallocation.  So, to make the ownership rules more explicit, I made the API take an already allocated char* and a size.
>> 
>> I wanted to allow for this idiom for people who don't want to guess the right string size:
>> 
>> size_t targetSize = LLVMGetProcessTarget(0, 0);
>> char *target = malloc(targetSize);
>> LLVMGetProcessTarget(target, targetSize);
>> 
>> This means that I couldn't just call snprintf().  I actually ended up getting the buffer allocation wrong on the first try, so I include some test coverage to make sure that it really isn't messed up.
>> 
>> -Filip
>> 
>> <GetProcessTarget.patch>
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130701/6fa73e4d/attachment.html>