Hi, I've been running LLVM with _GLIBCXX_DEBUG (extra checks) turned on to see what would happen, and it's been a complete disaster. The major problem is the use of this API: new CallInst(V, &Args[0], Args.size()); repeated throughout LLVM. When Args is empty, Args[0] is invalid, even if the next operation is taking the address. Trying to fix it illustrates the depth of the problem. Here are our choices: - Keeping the API a) Detect when the vector is empty and then pass a NULL pointer. This would require that the constructor accept a null pointer when the argument count is zero. b) Add an extra element to the vector, then subtract 1 from the size. This causes run time memory bloat. - Changing the API a) Template it to take two iterators. This causes code size bloat. b) Template it to take a container. This causes code size bloat. 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'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? Nick Lewycky
> > 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...#define V_CALLINST(V, Args) new CallInst(V, Args.size() == 0 ? NULL : &Args[0], Args.size()) :p -Keith
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. > > The major problem is the use of this API: > > new CallInst(V, &Args[0], Args.size()); > > repeated throughout LLVM. When Args is empty, Args[0] is invalid, even > if the next operation is taking the address.Ick.> Trying to fix it illustrates the depth of the problem. Here are our > choices: > > - Keeping the API > a) Detect when the vector is empty and then pass a NULL pointer. This > would require that the constructor accept a null pointer when the > argument count is zero.If the count is zero, the pointer is unused. The problem with this is that it would pollute the callers... :(> b) Add an extra element to the vector, then subtract 1 from the size. > This causes run time memory bloat.yuck. :(> - Changing the API > a) Template it to take two iterators. This causes code size bloat. > b) Template it to take a container. This causes code size bloat.'b' also prevents many important uses, like passing in a subset of a container.> 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 :(> 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... -Chris -- http://nondot.org/sabre/ http://llvm.org/
Chris Lattner wrote:> I don't really have a good idea, but I also don't want to significantly > uglify the sourcebase...The only one you didn't explicitly object to was templating on the iterators. That was my favourite idea. How bad is it to template here? Will we actually end up with two copies of each function or can the compiler collapse them? I'm not too familiar with optimizations at this level. I'd be willing to go through the errors reported by _GLIBCXX_DEBUG and fix them one at a time, but I don't want to experiment by going through all of them just to profile the difference. That's why I'm trying to decide the best fix (or whether to fix) up front. Nick
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
Nick Lewycky <nicholas at mxc.ca> writes:> Hi, > > I've been running LLVM with _GLIBCXX_DEBUG (extra checks) turned on to > see what would happen, and it's been a complete disaster. > > The major problem is the use of this API: > > new CallInst(V, &Args[0], Args.size());Forgive me if I'm missing something, but why is it assumed that &Args[0] must be a pointer to the entire contents of the vector? -- (let ((C call-with-current-continuation)) (apply (lambda (x y) (x y)) (map ((lambda (r) ((C C) (lambda (s) (r (lambda l (apply (s s) l)))))) (lambda (f) (lambda (l) (if (null? l) C (lambda (k) (display (car l)) ((f (cdr l)) (C k))))))) '((#\J #\d #\D #\v #\s) (#\e #\space #\a #\i #\newline)))))
On Tue, 3 Jul 2007, Jed Davis 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. >> >> The major problem is the use of this API: >> >> new CallInst(V, &Args[0], Args.size()); > > Forgive me if I'm missing something, but why is it assumed that > &Args[0] must be a pointer to the entire contents of the vector?This API works with sequential random access containers like vectors, arrays, smallvector, etc. It is not designed to work with arbitrary STL data structures, nor have we ever needed something like that. -Chris -- http://nondot.org/sabre/ http://llvm.org/