Valgrind buildbot failures

[David Blaikie]

Pity about not being able to use a reverse_iterator (& thus having to
duplicate bits of the implementation), but I'm OK with that.

On Sun, Aug 3, 2014 at 3:18 PM, Justin Bogner <mail at justinbogner.com> wrote:
> 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?
>
>
>
> 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
>