[llvm-commits] [llvm] r160751 - in /llvm/trunk: include/llvm/Analysis/MemoryBuiltins.h lib/Analysis/MemoryBuiltins.cpp lib/Transforms/Instrumentation/BoundsChecking.cpp test/Instrumentation/BoundsChecking/strings.ll

[John Criswell]

On 7/27/12 1:38 PM, Nuno Lopes wrote:
> Quoting Chandler Carruth <chandlerc at google.com>:
>
>> Sorry it's taken me a bit to circle back to this problem...
>>
>> On Wed, Jul 25, 2012 at 2:34 PM, Nuno Lopes <nunoplopes at sapo.pt> wrote:
>>
>>> Quoting Benjamin Kramer <benny.kra at gmail.com>:
>>>
>>>> On 25.07.2012, at 22:24, Nuno Lopes <nunoplopes at sapo.pt> wrote:
>>>>
>>>>> Quoting Duncan Sands <baldrick at free.fr>:
>>>>>
>>>>>> Hi Nuno,
>>>>>>
>>>>>>> original commit msg:
>>>>>>> MemoryBuiltins: add support to determine the size of strdup'ed
>>>>>>> non-constant strings
>>>>>> the dragonegg bots don't use cmake but they broke too:
>>>>>>
>>>>>> MemoryBuiltins.cpp:(.text+0x40dc): undefined reference to
>>>>>> `llvm::EmitStrNLen(llvm::Value*, llvm::Value*, llvm::IRBuilder<true,
>>>>>> llvm::ConstantFolder, llvm::IRBuilderDefaultInserter<true> >&,
>>>>>> llvm::TargetData
>>>>>> const*, llvm::TargetLibraryInfo const*)'
>>>>>> MemoryBuiltins.cpp:(.text+0x4207): undefined reference to
>>>>>> `llvm::EmitStrLen(llvm::Value*, llvm::IRBuilder<true,
>>> llvm::ConstantFolder,
>>>>>> llvm::IRBuilderDefaultInserter<true> >&, llvm::TargetData const*,
>>>>>> llvm::TargetLibraryInfo const*)'
>>>>> Interesting.. I guess I need to try to build it on another machine.
>>>> Isn't using lib/Transforms/Utils from lib/Analysis a layering
>>>> violation (cyclic dependency)?
>>> Well, I really need to access some functions from
>>> lib/Transforms/Utils. Is there any dependency of lib/Transforms/Utils
>>> on lib/Analysis?
>>>
>> No, you simply cannot depend on lib/Transforms code from within
>> lib/Analysis. That is a complete violation of the layering design here.
>>
>> I really don't have a clue on how to solve this problem.. :)
>> I think the fundamental problem is that ObjectSizeOffsetEvaluator isn't an
>> analysis, and shouldn't be in lib/Analysis at all. From its description:
>>
>> "Evaluate the size and offset of an object pointed [to] by a value*. May
>> create code to compute the result at run-time.". An Analysis pass shouldn't
>> create code. The only consumer of this interface is
>> lib/Transforms/Instrumentation/BoundsChecking.cpp, I think this class
>> should move to an anonymous namespace within that instrumentation pass. If
>> we want to hoist it, it could grow to live in
>> lib/Transforms/Utils/ObjectSize.cpp or even
>> lib/Transforms/Utils/MemoryBuiltins.cpp. I would only move it out of the
>> anonymous namespace if you have a pretty strong idea that others will want
>> to use the logic. I would only move it to a memory builtin utility file if
>> you think its likely we'll want more general memory builtin transform
>> utilities in the future.
>>
>> Can I make at least the move to the anonymous namespace? I'd like to get
>> the layering issue fixed.
> As John pointed out, there will be more clients for this analysis in
> the future.
> I agree it's not completely an analysis, but for example,
> Analysis/SCEVExpander isn't either  (it gets away, since it only uses
> IRBuilder).
> I put ObjectSizeOffsetEvaluator there, so that it could stay together
> with its cousin that analyzes buffers of constant sizes.
>
> I don't have any strong opinion about these types of things :)  So, if
> you want, we can move this class to lib/Transforms/Utils.

A few comments/thoughts:

1) Just in case people didn't catch it, the MemoryBuiltins code that 
Nuno is working on is not a pass but a utility library that passes can 
use.  Therefore, it is not breaking the rule that analysis passes not 
modify the LLVM IR.

2) Just an FYI that SAFECode's version of this library implements a 
version that does not modify the IR (called getAllocSize()) and a 
version that does modify the IR (getOrCreateAllocSize()).  Analysis 
passes use the former while transform passes use the latter; SAFECode 
needs both.  It sounds like your library is doing something similar.

3) It might make sense to break lib/Transforms/Utils into a general 
lib/Utility library that both Analysis and Transform passes can use.  It 
sounds like Nuno and I have both run into situations where a library 
needs to provide non-IR modifying and IR-modifying versions of the same 
functionality.  It sounds like ScalarEvolution does as well.

My two cents.

-- John T.



Nuno, does your library provide methods for finding the size of memory 
objects without changing the IR?  SAFECode provides
As far as "philosophical" opinions go, it might be better for the 
MemoryAnalysis pass to go into the
>
> Nuno
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits