Valgrind buildbot failures

[Justin Bogner]

I don't think this kind of stashing iterator is even a valid
bidirectional iterator. Nobody needs this to be bidirectional anyway
(everything iterates once through in one direction or the other) so I
guess the simplest fix is to split this into two input iterators like in
the attached.

WDYT?

-------------- next part --------------
A non-text attachment was scrubbed...
Name: path-reverse-iterator.patch
Type: text/x-patch
Size: 8297 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140803/fc30ba37/attachment.bin>
-------------- next part --------------

Tilmann Scheller <t.scheller at samsung.com> writes:
> 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
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits