Skip to content

Fix/verified issues - #215

Open
pereiraalessandra wants to merge 10 commits into
labmec:developfrom
pereiraalessandra:fix/verified-issues
Open

Fix/verified issues#215
pereiraalessandra wants to merge 10 commits into
labmec:developfrom
pereiraalessandra:fix/verified-issues

Conversation

@pereiraalessandra

@pereiraalessandra pereiraalessandra commented Jul 27, 2026

Copy link
Copy Markdown

Fix verified issues

Summary

This PR fixes eight issues identified by an AI-assisted code analysis.

For each issue, the sections below describe:

  • where the problem occurs;
  • why the current behavior is incorrect or unsafe;
  • what was changed.

1. SetTolerance() updated only one source file's copy of the variable

File: Topology/TPZTopologyUtils.h

Problem

gTolerance was defined as a namespace-scope static variable inside a header:

static REAL gTolerance = ...;

At namespace scope, static gives the variable internal linkage. Consequently, every .cpp file that includes the header gets its own independent copy of gTolerance, instead of all of them sharing one.

SetTolerance() is implemented in TPZTopologyUtils.cpp, so it only ever modified the copy that lives in that one .cpp file. Other users of gTolerance, including its use as the default tolerance in IsInParametricDomain(), are defined in different .cpp files and kept reading their own unchanged copies.

As a result, calls to SetTolerance() were silently ignored by most of the library.

Fix

Changed the definition to a C++17 inline variable:

inline REAL gTolerance = ...;

An inline variable may be defined in a header while still representing a single object shared by every .cpp file that includes it, instead of one copy per file.

2. Destroying a geometric mesh first left the computational mesh with a dangling pointer

File: Mesh/pzgmesh.cpp
Function: TPZGeoMesh::~TPZGeoMesh()

Problem

TPZCompMesh and TPZGeoMesh maintain references to each other.

When a computational mesh is destroyed first, TPZCompMesh::~TPZCompMesh() already notifies the associated geometric mesh by calling ResetReference().

The reverse case was not handled. TPZGeoMesh::~TPZGeoMesh() called CleanUp() without clearing the corresponding reference stored by the computational mesh.

If a geometric mesh was destroyed while an associated computational mesh was still alive, the computational mesh retained a pointer to freed memory. A subsequent access such as

TPZCompEl::Reference()

could eventually dereference the deleted geometric mesh through:

fMesh->Reference()->ElementVec()[...]

This resulted in undefined behavior.

Fix

Added the corresponding notification in TPZGeoMesh::~TPZGeoMesh():

if (fReference && fReference->Reference() == this) {
    fReference->SetReference(nullptr);
}

3. Debug checks used the wrong preprocessor macro

Files:

  • Shape/TPZShapeHDivConstant.cpp
  • Shape/TPZShapeHDivOptimized.cpp
  • Shape/TPZShapeHDiv.cpp
  • Shape/TPZShapeHDivConstantBound.cpp
  • Mesh/pzelchdiv.cpp
  • Mesh/pzelchdivbound2.cpp
  • Mesh/TPZCompElHDivDuplConnects.cpp
  • Mesh/TPZCompElHDivDuplConnectsBound.cpp
  • Material/Elasticity/TPZMixedElasticityND.cpp

Problem

The project uses PZDEBUG as its debug-build macro, but 11 consistency and bounds-checking blocks across these files used:

#ifdef DEBUG

Since DEBUG is not defined by the project build configuration, these blocks were never compiled, including in Debug builds.

Fix

Replaced all 11 occurrences with:

#ifdef PZDEBUG

4. TPZCreateApproximationSpace did not preserve its configured approximation families when copied

File: Pre/pzcreateapproxspace.h

Problem

The manually implemented copy constructor and assignment operator did not copy the following members:

fhdivfam
fh1fam
fhcurlfam
fStyle

These members record the configured approximation-space families and style.

TPZCompMesh contains a TPZCreateApproximationSpace object and exposes mesh cloning through Clone(). Therefore, cloning a mesh configured with a non-default family, such as EHDivConstant, could produce an object whose stored configuration reported the default family even though its shape-function creation callbacks still reflected the original configuration.

This left the copied object in an internally inconsistent state.

Fix

Replaced the hand-written copy operations with compiler-generated ones:

TPZCreateApproximationSpace(
    const TPZCreateApproximationSpace &copy) = default;

TPZCreateApproximationSpace &operator=(
    const TPZCreateApproximationSpace &copy) = default;

All members already have valid copy semantics. Using = default also prevents future data members from being accidentally omitted from the copy operations.

Also cleaned up three setters in the same header that had const as part of a void return type, e.g.:

const void SetHDivFamily(HDivFamily fam){ ... }

changed to:

void SetHDivFamily(HDivFamily fam){ ... }

const has no effect on a void return type. This does not change how these methods behave or are called.

5. TPZAnalysis could be shallow-copied, causing double deletion of fSolver

Files:

  • Analysis/TPZAnalysis.h
  • Post/pzpostprocanalysis.h
  • Post/pzpostprocanalysis.cpp

Problem

class TPZAnalysis {
    TPZSolver *fSolver;
    void CleanUp() { delete fSolver; }
    ~TPZAnalysis() { CleanUp(); }
    // no copy constructor / operator= declared
};

TPZAnalysis owns fSolver: it deletes it during cleanup and in its destructor. But the class did not declare a copy constructor or operator=, so the compiler generated its own. For a raw pointer member, that means copying the address, not the object it points to:

TPZAnalysis a1;
a1.SetSolver(mySolver);   // a1.fSolver points to address X

TPZAnalysis a2 = a1;      // a2.fSolver ALSO points to address X

a1 and a2 are now two separate objects whose fSolver points to the same TPZSolver. Each one still believes it owns that solver and will delete it when destroyed. The one that is destroyed second ends up calling delete on memory that the first one already freed (double free).

TPZPostProcAnalysis (a subclass of TPZAnalysis) also explicitly wrote its own copy constructor and operator=, instead of just inheriting the unsafe compiler-generated ones:

TPZPostProcAnalysis::TPZPostProcAnalysis(const TPZPostProcAnalysis &copy)
    : TPZLinearAnalysis(copy), fpMainMesh(0)
{
}

TPZPostProcAnalysis &TPZPostProcAnalysis::operator=(const TPZPostProcAnalysis &copy)
{
    SetCompMesh(0);
    return *this;
}

The copy constructor calls TPZLinearAnalysis(copy), which copies the base class TPZAnalysis and inherits the same double-free problem. The operator= does not copy anything from copy at all (it just resets the current object and ignores its argument).

Fix

Both classes now explicitly forbid copying, instead of allowing an unsafe copy or hand-writing a broken one:

TPZAnalysis(const TPZAnalysis &) = delete;
TPZAnalysis &operator=(const TPZAnalysis &) = delete;
TPZPostProcAnalysis(const TPZPostProcAnalysis &copy) = delete;
TPZPostProcAnalysis &operator=(const TPZPostProcAnalysis &copy) = delete;

= delete tells the compiler to reject any attempt to copy one of these objects.

No code in the current repository copies a TPZAnalysis or a TPZPostProcAnalysis, so this change does not affect any existing behavior. It only removes an operation that was never safe to use.

6. Wrong face index in the Fad<REAL> branch of TPZShapeHDivConstant

File: Shape/TPZShapeHDivConstant.cpp

Problem

The Fad<REAL> overload of TPZShapeHDivConstant::Shape() used:

data.fHCurl.fNumConnectShape[nedges]

inside a loop over the element facets.

Because the index did not include the current facet index i, every facet used the kernel-function count associated with the first facet.

The corresponding REAL overload already used the correct expression:

data.fHCurl.fNumConnectShape[nedges + i]

This can misalign the function blocks and lead to incorrect shape functions, incorrect automatic derivatives, or out-of-bounds matrix accesses.

Fix

Changed:

data.fHCurl.fNumConnectShape[nedges]

to:

data.fHCurl.fNumConnectShape[nedges + i]

The Fad<REAL> branch now matches the indexing already used by the REAL branch.

7. Tautological condition in TPZH1ApproxCreator::CreateBoundaryHDivSpace()

File: Pre/TPZH1ApproxCreator.cpp

Problem

The function contained the following condition:

if (fHybridType != HybridizationType::EStandard ||
    fHybridType != HybridizationType::EStandardSquared) {

This expression is always true.

For any possible value of fHybridType, it must be different from at least one of the two enum values. Consequently, the corresponding else branch containing DebugStop() was unreachable.

Fix

Changed the comparisons from != to == while preserving ||:

if (fHybridType == HybridizationType::EStandard ||
    fHybridType == HybridizationType::EStandardSquared) {

With the current call flow, fHybridType can only be EStandard or EStandardSquared when execution reaches this function. CheckSetupConsistency() already rejects ESemi, and CreateAtomicMeshes() rejects ENone.

Therefore, this change does not alter the behavior of any currently valid execution path. It only serves as a defensive check, ensuring that any future unsupported or unhandled enum value is detected instead of being silently treated as valid.

8. Missing braces made the control flow in CreateAtomicMeshes() misleading and fragile

File: Pre/TPZH1ApproxCreator.cpp

Problem

The function contained:

if (HybridType() != HybridizationType::ENone)
    meshvec[countMesh++] = CreateBoundaryHDivSpace();
    meshvec[countMesh++] = CreateL2Space();

In C++, an if statement without braces controls only the immediately following statement.

Therefore, despite the indentation, the code was interpreted as:

if (HybridType() != HybridizationType::ENone) {
    meshvec[countMesh++] = CreateBoundaryHDivSpace();
}

meshvec[countMesh++] = CreateL2Space();

The current behavior is valid because ENone is rejected earlier in the function. However, the misleading indentation made the intended control flow unclear and left the code dependent on that earlier validation remaining unchanged.

Fix

Added braces around the statement that is actually conditional:

if (HybridType() != HybridizationType::ENone) {
    meshvec[countMesh++] = CreateBoundaryHDivSpace();
}

meshvec[countMesh++] = CreateL2Space();

This change clarifies the actual behavior without changing it.

@orlandini

orlandini commented Jul 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Hello,

  1. Have you verified that this behaviour indeed occurs? I have performed a small check and it seems like it is working as it should.
  2. Seems fine
  3. Seems fine
  4. Seems fine
  5. If that is the intended behaviour, then it is fine.
  6. Seems fine, should have been tested. Are there unit tests?
  7. The current code doesn't make sense, indeed. What is the desired behaviour?
  8. The code is actually properly indented, but the added braces are a good choice.

@philippedevloo

philippedevloo commented Jul 28, 2026 via email

Copy link
Copy Markdown
Member

@orlandini

orlandini commented Jul 28, 2026

Copy link
Copy Markdown
Member

@philippedevloo ,

I did a really small test:

main.cpp:

...
int main(){
  pztopology::SetTolerance(1e-2);
  std::cout<<"Tolerance in main file "<<pztopology::GetTolerance()<<std::endl;
   ...
  wgma::util::CreatePath(wgma::util::ExtractPath(simdata.prefix));
  ...
}

wgma/util.cpp:

  void CreatePath(const std::string filepath){

    std::cout<<"Tolerance in util file "<<pztopology::GetTolerance()<<std::endl;
    auto path = ExtractPath(filepath);
 
    if(path.size() != 0){
      CreatePath(path);
    }
    std::filesystem::create_directory(filepath);
  }

And I have obtained the same values:

Tolerance in main file 0.01
Using the following NeoPZ log config file:
/opt/neopz/pz/include/Util/log4cxx.cfg
Tolerance in util file 0.01

But I confess that I do not know if this test was enough.

@orlandini orlandini closed this Jul 28, 2026
@orlandini orlandini reopened this Jul 28, 2026
@pereiraalessandra

pereiraalessandra commented Jul 28, 2026

Copy link
Copy Markdown
Author

@orlandini

I think the test doesn't catch it because both prints call GetTolerance(). This function has only one definition, in TPZTopologyUtils.cpp, so every call reads the same instance of gTolerance, regardless of which source file calls it.

The bug becomes visible when gTolerance is accessed directly from a source file other than TPZTopologyUtils.cpp, without going through GetTolerance(). One example already in the codebase:

  • Topology/tpzprism.cpp:990: double zero = pztopology::gTolerance;, inside TPZPrism::CheckProjectionForSingularity.

To reproduce the problem, change the print in wgma/util.cpp from

pztopology::GetTolerance()

to a direct access:

pztopology::gTolerance

Since wgma/util.cpp and TPZTopologyUtils.cpp are compiled separately, the print in wgma/util.cpp should then show the default value instead of 0.01.

It would also be useful to add unit tests covering this problem and the behavior reported in issue 6.

@orlandini

orlandini commented Jul 28, 2026 via email

Copy link
Copy Markdown
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@pereiraalessandra @orlandini @philippedevloo