[dune-fem] Bug in jacobianAll

[Martin Nolte]

Hi Marco,

On 03/05/2015 12:45 PM, Agnese, Marco wrote:
> Hi Martin,
>
>> Well, with the copyable entities, writing *entityIterator _and_ then storing a
>> reference to its result is the only true source of segfaults I know of. In
>> itself, both operations are ok, but they may not be combined.
>
> I don't understand why.
> I mean dereferencing an iterator gives you an entity and simply you pass the reference to that entity to the method.
> Am I missing something?

This is a tricky one. There is a new feature in dune-grid: Iterators no longer 
have to return a reference to the entity they are pointing. Instead they may 
return a copy of the entity object. GeometryGrid, for example, uses this feature 
already. Now, if you dereference the iterator, you get a newly created temporary 
entity object. Now assume you write something like:

LocalFunction lf = df.localFunction( *it );

This passes a reference to the temporary entity object (*it) to the 
localFunction lf, which stores this reference internally (in the 
BasisFunctionSet). Now, C++ storage rule imply that the temporary object (*it) 
may be destructed after the statement has been executed (i.e., after the 
semicolon). From now on, lf contains a dangling reference. Note that newer 
versions of gcc agressivly use this possibility to optimize the generated code.

Now, things are completely different if you write

const Entity &e = *it;
LocalFunction lf = df.localFunction( e );

In this case, the temporary returned by (*it) is destructed at the end of the 
enclosing block, where all local variables include lf are destroyed. The reason 
for this behavior is that you named the temporary (here: 'e'). As a result, 
there is no dangling reference. Note that, if the iterator returns the entity by 
value, it does not make a notable difference whether you write

const Entity &e = *it;

or

const Entity e = *it;

A new copy of the entity is constructed either way (there might be a small 
difference in performance for some grids overloading the copy constructor, though).

I have to admit that you really have to read the C++ standard to realize this 
subtle difference. Unfortunately, it would require sophisticated C++ magic to 
warn you against this pitfall. Such problems are actually the reason why I would 
have preferred the hard break in compatibility.

For the time being, I can only urge people to name the entity before passing it 
on. This gives you basically the same lifetime guarantees you had before when 
the entity was actually stored in the iterator (I know of no grid implementation 
that ever kept all entities in memory. Actually I do not even have a clue how to 
do that).

Best,

Martin

>
>> So: Stick to the paradigm we successfully used over the last years: First
>> defererence the entity iterator and store (a reference to) the entity. Then pass
>> this entity around and you're basically safe. Just make sure the entity lives
>> longer than any object (e.g., LocalFunction, LocalMatrix, etc.) associated to it.
>
> Yes, I did it :)
>
>> A nice side effect of range-based for loops is that you are automatically safe
>> as long as you don't propagate objects like LocalFunctions out of the loop
>> (which I have never seen in dune-fem).
>
> I have to start to use the range-based loop!
>
>> Should we put such information onto some FAQ list or into the release notes?
>
> I think that a note in the release notes would be useful and may save a lot of time since it is a tricky bug to spot.
>
> Best,
>
> Marco
> _______________________________________________
> dune-fem mailing list
> dune-fem at dune-project.org
> http://lists.dune-project.org/mailman/listinfo/dune-fem
>

-- 
Dr. Martin Nolte <nolte at mathematik.uni-freiburg.de>

Universität Freiburg                                   phone: +49-761-203-5630
Abteilung für angewandte Mathematik                    fax:   +49-761-203-5632
Hermann-Herder-Straße 10
79104 Freiburg, Germany