[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

[Nuno Lopes]

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.

Nuno