Valgrind buildbot failures

[Tilmann Scheller]

Hi,

I found the bug: sys::path::const_iterator is a "stashing iterator" (an iterator which refers to data inside itself) and those are not allowed to be used with std::reverse_iterator (see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51823 or https://connect.microsoft.com/VisualStudio/feedback/details/813613/tempories-created-by-std-reverse-iterator-return-garbage-values-with-certain-base-iterators)

Here's the code:

class const_iterator {
  StringRef Path;      ///< The entire path.
  StringRef Component; ///< The current component. Not necessarily in Path.
  size_t    Position;  ///< The iterators current position within Path.
...
  reference operator*() const { return Component; }
  pointer   operator->() const { return &Component; }
...
};

typedef std::reverse_iterator<const_iterator> reverse_iterator;

This is the operator* implementation of reverse_iterator in libstdc++:

reference
operator*() const
{
  _Iterator __tmp = current;
 return *--__tmp;
}

As you can see we return a dangling reference to Current in the temporary iterator object.

Looks like this just works because the stack is not getting clobbered quickly enough.

The interesting questions is how to fix this :)

To me it's not obvious how we could change the iterator to stop it from being a "stashing iterator" (unless I'm missing something here).

Maybe just change the users of reverse_iterator? It looks like there's only a single use (in the YAML parser).

Regards,

Tilmann

-----Original Message-----
From: David Blaikie [mailto:dblaikie at gmail.com] 
Sent: Friday, July 25, 2014 5:21 PM
To: Tilmann Scheller
Cc: llvm-commits at cs.uiuc.edu
Subject: Re: Valgrind buildbot failures

Thanks for taking a look - hope you find it. I'd be happy to pitch in/give you ideas as you go if you have any questions.

A cursory inspection seems to indicate that we use std::reverse_iterator for rend/rbegin, so I suspect the bug isn't in the reverse iterator itself, but just in the implementation of sys::path::iterator::operator--. Changing from an rbegin/rend loop, to
this:

for (iterator i = end, b = begin; i != b; --i) {
  ... std::prev(i) ...
}

or similar, if it still reproduces the bug then that should support my hypothesis and help reduce your search space a bit, at least.

On Fri, Jul 25, 2014 at 8:07 AM, Tilmann Scheller <t.scheller at samsung.com> wrote:
> Hi,
>
> the Valgrind builbot (http://lab.llvm.org:8011/builders/clang-x86_64-linux-vg) has been red for quite some time, I found some time today to start looking into the failures.
>
> The majority of the failures seems to be an instance of this reduced test case:
>
> #include <cstdio>
> #include <llvm/Support/Path.h>
> #include <llvm/ADT/StringRef.h>
>
> using namespace llvm;
>
> int main(int argc, char** argv) {
>   StringRef str("/");
>
>   for (sys::path::reverse_iterator I = sys::path::rbegin(str),
>                                    E = sys::path::rend(str);
>        I != E; ++I) {
>     StringRef tmp = *I;
>     printf("str: %p %s %lu\n",  &tmp, tmp.data(), tmp.size());
>   }
>
>   return 0;
> }
>
> Compiling this with clang at -O1 results in the following errors reported by Valgrind:
>
> ==2649== Conditional jump or move depends on uninitialised value(s)
> ==2649==    at 0x32AE448EE4: vfprintf (in /usr/lib64/libc-2.18.so)
> ==2649==    by 0x32AE451F28: printf (in /usr/lib64/libc-2.18.so)
> ==2649==    by 0x403BD4: main (in /home/t/work/arm/llvm/a.out)
> ==2649==
> ==2649== Use of uninitialised value of size 8
> ==2649==    at 0x32AE4490CF: vfprintf (in /usr/lib64/libc-2.18.so)
> ==2649==    by 0x32AE451F28: printf (in /usr/lib64/libc-2.18.so)
> ==2649==    by 0x403BD4: main (in /home/t/work/arm/llvm/a.out)
> ==2649==
> ....
>
> I suspect that somewhere in the iterator we have some off by one bug. Changing the iterator to a forward iterator doesn't trigger the bug.
>
> I'll probably look deeper into this next week, just sharing my initial results here hoping that someone more familiar with the sys::path iterator code can spot the bug before I do.
>
> Regards,
>
> Tilmann
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits