Segmentation fault in cell.facet_area under certain conditions
I get a segmentation fault when running cell.facet_area(0)
in both FEniCS 1.5 and 1.6
from dolfin import *
#parameters['reorder_dofs_serial'] = False
mesh = UnitSquareMesh(4, 4, 'right')
#V = FunctionSpace(mesh, 'DG', 1)
cell = Cell(mesh, 0)
cell.facet_area(0)
If I uncomment the "V = ..." line the segfault goes away. If I further uncomment the reorder_dofs line it again segfaults.
Comments (23)
-
-
reporter Yep, that works.
I assumed this was related to some data structure not being initialized before building a function space, it was the interplay with reorder_dofs_serial that took me a while to figure out ...
-
Actually it segfaulted for me regardless of reorder_dofs_serial, I think that was just random.
-
reporter For both me and Miro the following works:
from dolfin import * #parameters['reorder_dofs_serial'] = False mesh = UnitSquareMesh(4, 4, 'right') V = FunctionSpace(mesh, 'DG', 1) cell = Cell(mesh, 0) cell.facet_area(0)
While this segfaults.
from dolfin import * parameters['reorder_dofs_serial'] = False mesh = UnitSquareMesh(4, 4, 'right') V = FunctionSpace(mesh, 'DG', 1) cell = Cell(mesh, 0) cell.facet_area(0)
The latter example is how I found this bug in the first place.
-
I'm not sure how to classify this. The solution is to do mesh.init(1), maybe we just need to document that, and add an assertion to Cell::facet_area.
-
I agree - document it, plus an assertion.
-
- removed milestone
Removing milestone: 1.7 (automated comment)
-
reporter I looked into this and I think the problem is that cell.entities(1) returns an empty list which the cell implementation (TriangleCell.cpp) tries to use to look up the facet
// Create facet from the mesh and local facet number const Facet f(cell.mesh(), cell.entities(1)[facet]);
Should cell.entities() in MeshEntity.h check to see it the connectivity has been initialized so that some errors can be avoided when it returns an empty set? The current implementation is just
const unsigned int* entities(std::size_t dim) const { return _mesh->topology()(_dim, dim)(_local_index); }
-
reporter This change to MeshEntity.h gives an explanatory error message when asking for non-initialized entities, but other places may rely on the empty result somehow?
const unsigned int* entities(std::size_t dim) const { auto& mesh_connectivity = _mesh->topology()(_dim, dim); if (mesh_connectivity.empty()) { dolfin_error("MeshEntity.h", "get entities", "The mesh connectivity is empty (not initialized). " "Run mesh.init(%d) to initialize connectivity", dim); } return mesh_connectivity(_local_index); }
-
This could be a hot spot. Maybe just
dolfin_assert
is appropriate here. -
Agree - this should be a
dolfin_assert
. -
reporter Ok, I guess an error message with !mesh_connectivity.empty() is explanatory enough
-
reporter Could I get added to the team so that I can make a branch directly in dolfin so that the unit tests are run? I will still work through pull requests. It is just a hassle to keep a fork just to make small fixes like this
-
reporter - changed status to resolved
Assert that mesh connectivity is present before returning entities
This fixes issue
#568.→ <<cset 8ab4318892a9>>
-
- changed status to open
Not yet in master. Do you have access to
master
andnext
, @trlandet? -
-
assigned issue to
-
assigned issue to
-
@trlandet it seems that 8ab4318892a9 does not pass the tests http://fenics-bamboo.simula.no:8085/browse/DOL-DODO75. Can you fix it?
-
reporter Back from vacation now.
@blechta: I have merged master into the branch to see if this fixes the test failures. When I made the fix three weeks ago it seemed that every branch was failing with some kind of Instant error, so it might have been something unrelated. Lets see.
I do not think I can change master or next currently so I will ask for someone else to merge it into next (or should I create pull requests towards next and then master?)
-
Create a PR targeting
master
. -
I didn't think that such a minimal change needs a PR. That's why I was asking whether you have the access.
Anyway the failure seems to be really cause by this change. The assertion fails in unit tests and demos. (Instant stderr message on "Job summary" tab is misleading. This one is, I think, expected to happen and caught in a unit test.) Real problem is visible on "Logs" tab, see http://fenics-bamboo.simula.no:8085/browse/DOL-DODO75-PUT-33/log.
You can follow example at https://bitbucket.org/fenics-project/testing/issues/1 to fetch the build and reproduce the problem with essentially no work.
-
To the actual problem, maybe you should rather test that you're not dereferencing nullptr or indexing out of bounds. Now you asking whether the connectivity is empty.
-
reporter The code now checks for null pointer and fixes the one place in the DOLFIN code that relied on getting a null pointer and not using it. At least, that was the only place covered by the tests. I also added a unit test for this issue
-
reporter - changed status to resolved
Fixed in pull request #291
- Log in to comment
With latest dev and mesh = UnitSquareMesh(1,1) it also fails.
Adding mesh.init(1) (which initializes the edges of the mesh) makes it pass, which I believe is hinting at the problem. Can you try that on your installation?
@logg