On Monday 02 July 2007 16:26, Chris Lattner wrote:> On Sun, 1 Jul 2007, Nick Lewycky wrote: > > I've been running LLVM with _GLIBCXX_DEBUG (extra checks) turned on to > > see what would happen, and it's been a complete disaster.Well, that's a bit harsh, isn't it? It's finding bugs, just like it's supposed to. :) I believe I've started to run into this one too. I hit a similar error and was in the middle of tracking down the source.> > - Changing the API > > a) Template it to take two iterators. This causes code size bloat.This seems like the right solution to me. Unless llvm is running on extremely limited memory embedded systems, the extra code space shouldn't be an issue.> > We could also just ignore the problem. I'm not clear on what the effects > > of that would be in the long term. Is this something we can expect will > > be changed in C++0x? > > I don't think that this issue causes any problems in practice. The > biggest bad thing is that it makes the extra checking code apparently > useless :(The index operation can imply a dereference on some implementations, otherwise the checking code wouldn't have caught it. In other words, the standard requires that the memory being indexed by referenceable. Violating that could cause all sorts of interesting things, like segmentation faults or the sudden sprouting of rose bushes on Dan Gohman's head. I agree that on common architectures it probably won't cause a problem. But it still results in undefined behavior.> > I'd like to get the extra checks working so that they can help find our > > more subtle bugs. Any idea what we should do here? > > I don't really have a good idea, but I also don't want to significantly > uglify the sourcebase...Passing two iterators is in the spirit of the standard library, so I don't consider that uglification. I understand others may disagree. In any event, leaving this kind of code in is asking for trouble down the road. -Dave
On Mon, 2 Jul 2007, David A. Greene wrote:>>> - Changing the API >>> a) Template it to take two iterators. This causes code size bloat. > > This seems like the right solution to me. Unless llvm is running on > extremely limited memory embedded systems, the extra code space > shouldn't be an issue.Code size is always an issue. What specifically are you proposing? I assume you want to eliminate/supplement this ctor: CallInst::CallInst(Value *Func, Value* const *Args, unsigned NumArgs, const std::string &Name, BasicBlock *InsertAtEnd) Turning it into a template would require duplicating it (small), and CallInst::init (large). I don't really think this is workable.>>> We could also just ignore the problem. I'm not clear on what the effects >>> of that would be in the long term. Is this something we can expect will >>> be changed in C++0x? >> >> I don't think that this issue causes any problems in practice. The >> biggest bad thing is that it makes the extra checking code apparently >> useless :( > > The index operation can imply a dereference on some implementations, > otherwise the checking code wouldn't have caught it. In other words, the > standard requires that the memory being indexed by referenceable. Violating > that could cause all sorts of interesting things, like segmentation faults or > the sudden sprouting of rose bushes on Dan Gohman's head. > > I agree that on common architectures it probably won't cause a problem. > But it still results in undefined behavior.I agree about undefined behavior. As I said above, I don't think it causes any problems on any implementations we have seen, nor is it likely to in the future. That said, I obviously agree that relying on undefined behavior is bad :)>>> I'd like to get the extra checks working so that they can help find our >>> more subtle bugs. Any idea what we should do here? >> >> I don't really have a good idea, but I also don't want to significantly >> uglify the sourcebase... > > Passing two iterators is in the spirit of the standard library, so I don't > consider that uglification. I understand others may disagree.I totally agree that it is clean, the question is, how do we make it work? Here's a different suggestion that cloning all the code. Instead of doing that, why not add a new (templated) CallInst ctor, one which is very trivial. Then you could put the conditional code in it (to detect an empty range) and call into the non-inline stuff we already have. It should be fine to only support random access iterators, so you could call into the existing stuff that takes a base + size. -Chris -- http://nondot.org/sabre/ http://llvm.org/
On Tuesday 03 July 2007 00:44, Chris Lattner wrote:> On Mon, 2 Jul 2007, David A. Greene wrote: > >>> - Changing the API > >>> a) Template it to take two iterators. This causes code size bloat. > > > > This seems like the right solution to me. Unless llvm is running on > > extremely limited memory embedded systems, the extra code space > > shouldn't be an issue. > > Code size is always an issue.To a greater or lesser degree. Context matters.> Here's a different suggestion that cloning all the code. Instead of doing > that, why not add a new (templated) CallInst ctor, one which is very > trivial. Then you could put the conditional code in it (to detect an > empty range) and call into the non-inline stuff we already have. > > It should be fine to only support random access iterators, so you could > call into the existing stuff that takes a base + size.That's pretty close to what I was thinking. I think it's a good solution. -Dave
On Monday 02 July 2007 23:24, David A. Greene wrote:> > > - Changing the API > > > a) Template it to take two iterators. This causes code size bloat. > > This seems like the right solution to me. Unless llvm is running on > extremely limited memory embedded systems, the extra code space > shouldn't be an issue.It turns out this wasn't quite a simple as we'd thought. The problem is the CallInst constructors that take two Value * arguments. They look just like the teamplte versions that take iterators. Unfortunately, there is at least one place in llvm where a CallInst is constructed with two AllocaInst pointers. Because the template constructor is a better match, it is selected over the Value * constructors. To get around this problem, I've used SFINAE to remove the template constructors from the overload set when passed pointers to classes derived from Value: namespace detail { template<bool value> struct disable_if {}; template<> struct disable_if<false> { typedef int type; }; } /// CallInst - This class represents a function call, abstracting a target [...] /// Construct a CallInst given a range of arguments. InputIterator /// must be a random-access iterator pointing to contiguous storage /// (e.g. a std::vector<>::iterator). Checks are made for /// random-accessness but not for contiguous storage as that would /// incur runtime overhead. /// @brief Construct a CallInst from a range of arguments template<typename InputIterator> CallInst(Value *Func, InputIterator ArgBegin, InputIterator ArgEnd, const std::string &Name = "", Instruction *InsertBefore = 0, // This extra default argument use SFINAE to prevent the // template from being considered for pointers to // Value-derived types. It removes an unwanted // instantiation when the caller really wants one of the // Value * versions below. Because Value is a base class, // the template match is preferred for classes derived from // Value if this argument is not here. Note that if // InputIterator IS Value *, the non-template constructor // is a better match. typename detail::disable_if< is_base_and_derived< Value, typename remove_pointer<InputIterator>::type >::value >::type enabler = 0) : Instruction(cast<FunctionType>(cast<PointerType>(Func->getType()) ->getElementType())->getReturnType(), Instruction::Call, 0, 0, InsertBefore) { init(Func, ArgBegin, ArgEnd, Name, typename std::iterator_traits<InputIterator>::iterator_category()); } init() is the function that actually does the checking of the iterators for an empty range. This required that I pull in is_base_and_derived, remove_pointer, is_same and remove_cv from Boost. boost::is_class already exists in Support/type_traits.h so there is precedent for this. Now, something is not quite right with the code as the overload doesn't seem to get removed when passed an AllocaInst *. I'm working on figuring out why. I wrote a small testcase which passes. I'm working on reducing UpgradeParser.cc to a minimal testcase that shows the problem. I'm wondering if I've stumbled on a gcc bug. Is this an acceptable solution to the problem? Is importing these Boost components ok? tr1 includes all of the Boost type traits so another option is to just use those and impose a requirement that compilers used to build llvm must support tr1 type traits. GNU libstdc++ already does. -Dave
On Wed, 4 Jul 2007, David A. Greene wrote:> On Monday 02 July 2007 23:24, David A. Greene wrote: >>>> - Changing the API >>>> a) Template it to take two iterators. This causes code size bloat. >> >> This seems like the right solution to me. Unless llvm is running on >> extremely limited memory embedded systems, the extra code space >> shouldn't be an issue. > > It turns out this wasn't quite a simple as we'd thought. The problem is > the CallInst constructors that take two Value * arguments. They look > just like the teamplte versions that take iterators.Wow, this sounds very complex :)> template<typename InputIterator> > CallInst(Value *Func, InputIterator ArgBegin, InputIterator ArgEnd, > const std::string &Name = "", Instruction *InsertBefore = 0,Is it acceptable to just make the template argument be the container? That way you could pass: std::vector.. V; ... new CallInst(Callee, V, "foo"); etc. The disadvantage is that you lose the ability to pass a subrange. However, if you know you're using a subrange, you should be able to use another ctor form. -Chris -- http://nondot.org/sabre/ http://llvm.org/