mgehre / llvm-project

The home of the clang-based implementation of lifetime safety warnings.
39 stars 4 forks source link

How do I properly dereference an iterator? #94

Open davidwin opened 4 years ago

davidwin commented 4 years ago

The -Wlifetime project is intriguing, but there are still too many false positives in real-world projects I have tested it on. One thing I quite haven't understood is how to treat iterators in order not to trigger null dereference warnings. Very often in generic iterator code, you don't compare an iterator against a null value before dereferencing it, but against the end of a range. Do I need to annotate iterator code in any way to avoid diagnostics? For instance, consider this silly function that just returns the value of the first integer in a range or zero if the range is empty:

template<typename It>
int check(It begin, It end)
{
    return begin < end ? *begin : 0;
}
...
// Called from somewhere
std::array x { 1, 2, 3 };
int y = check(x.begin(), x.end());

This results in the following diagnostics:

warning: dereferencing a possibly null pointer [-Wlifetime-null]
    return begin < end ? *begin : 0;
                         ^~~~~~
note: in instantiation of function template specialization 'check<int *>' requested here
    return check(x.begin(), x.end());
           ^
note: the parameter is assumed to be potentially null. Consider using gsl::not_null<>, a reference instead of a pointer or an assert() to explicitly remove null
int check(It begin, It end)
          ^~~~~~~~

Now, not_null<> doesn't feel like a good option here, and depending on the iterator, it can't be checked against null before dereferencing. In the case of std::array, this would work:

template<typename It>
int check(It begin, It end)
{
    return begin && begin < end ? *begin : 0;
}

It doesn't look like ideomatic iterator code, though. And it wouldn't compile if std::vector iterators were used. Comparing against a default constructed iterator technically works for some more iterator types, but it doesn't remove the -Wlifetime-null diagnostic for std::vector iterators. See an example here: https://godbolt.org/z/aUCm3D

I suppose that I have missed something fundamental here. I would like to be reminded if I dereference an iterator with comparing it to something, but sometimes and end iterator is the "null value".

mgehre commented 4 years ago

Thanks for taking the time to report this issue!

You are right, the null diagnostics together with iterations don't make a lot of sense currrently. Conceptually, the end iterator is like the null value: It's a value that a) cannot be dereferenced and b) one can check that another iterator is equal to it. Unfortunately, the implementation isn't good enough here.

We might generally want to only emit null diagnostics when we are sure that there is a null/end, and not just because there could be.

In the meantime, I suggest to use -Wno-lifetime-null when looking at real-world code. You can additionally try -Wlifetime-filter, which also reduced the number of the other lifetime warnings to reduce false-positives.

davidwin commented 4 years ago

I hope it will be possible to make good iterator analysis eventually, but I can see it must be tricky if analysis is only limited to a single function scope and shouldn't have to rely on heavy annotations. With sentinel iterators, how is the analysis even supposed to guess to what you should be comparing the dereferenced iterator? Maybe if you compare it to something in one branch before dereferencing, that is the "null", and you should get a diagnostics if you forget the check in some other branch.

And what about output iterators? They are a bit of a scary concept in general (e.g. a raw pointer that is written to and incremented without any checks at all), but then there are safe output iterators that merely acts as data sinks. I hope you come up with some great solution!