Clang tidy CI test: add several clang-analyzer-* checks to clang tidy CI test #4684

lucafedeli88 · 2024-02-09T09:00:03Z

This PR adds several clang-analyzer-* checks to clang-tidy CI test:

The following checks are explicitly excluded (osx checks are not relevant for WarpX):

clang-analyzer-osx.*
clang-analyzer-optin.osx.*

Also this check is excluded, since it generates a non-relevant warning:

clang-analyzer-optin.mpi.MPI-Checker

Finally, padding check is excluded because it generates warning for lambda functions, which require a verbose NOLINT to be silenced:

clang-analyzer-optin.performance.Padding

Notable changes

Use new `AMREX_ASSUME` feature to silence warnings

In WarpX we can enforce a given condition by using WARPX_ALWAYS_ASSERT_WITH_MESSAGE . However, the compiler can't see through WARPX_ALWAYS_ASSERT_WITH_MESSAGE and clang-tidy may therefore generate warnings for scenarios that can't actually take place.

A possible solution is to use a newly introduced feature in AMReX: AMREX_ASSUME :

#include <AMReX_Extension.H>
//...
WARPX_ALWAYS_ASSERT_WITH_MESSAGE(m_mf_src != nullptr, "m_mf_src can't be a nullptr.");
AMREX_ASSUME(m_mf_src != nullptr);

After AMREX_ASSUME(m_mf_src != nullptr); most compilers (including gcc, clang, and nvcc) should be able to assume that m_mf_src != nullptr.

Using this feature requires to update the AMReX version.

Change getters of fields and field pointers in WarpX

As discussed with @ax3l (see #4684 (comment)), this PR also changes the getters of the fields and field pointers in the WarpX class:

enum struct FieldType : int
{
    Efield_aux,
    Bfield_aux,
    Efield_fp,
    Bfield_fp,
    current_fp,
    current_fp_nodal,
    rho_fp,
    F_fp,
    G_fp,
    phi_fp,
    vector_potential_fp,
    Efield_cp,
    Bfield_cp,
    current_cp,
    rho_cp,
    F_cp,
    G_cp,
    edge_lengths,
    face_areas,
    Efield_avg_fp,
    Bfield_avg_fp,
    Efield_avg_cp,
    Bfield_avg_cp
};

//...

[[nodiscard]] bool
isFieldInitialized (FieldType field_type, int lev, int direction = 0) const;

[[nodiscard]] amrex::MultiFab*
getFieldPointer (FieldType field_type, int lev, int direction = 0) const;

[[nodiscard]] std::array<const amrex::MultiFab* const, 3>
getFieldPointerArray (FieldType field_type, int lev) const;

const amrex::MultiFab&
getField(FieldType field_type, int lev, int direction = 0) const;

//...
private:

[[nodiscard]] amrex::MultiFab*
getFieldPointerUnchecked (FieldType field_type, int lev, int direction = 0) const;

getFieldPointerUnchecked does not check if the requested pointer is != nullptr . This function is conceived to be used internally by the other getters, which check if the pointer is valid and, in case it isn't, they abort.

… CI test

ax3l · 2024-02-13T00:07:15Z

Source/Diagnostics/ComputeDiagFunctors/JFunctor.cpp

@@ -54,6 +54,12 @@ JFunctor::operator() (amrex::MultiFab& mf_dst, int dcomp, const int /*i_buffer*/
        mypc.DepositCurrent(current_fp_temp, warpx.getdt(m_lev), 0.0);
    }

-    InterpolateMFForDiag(mf_dst, *m_mf_src, dcomp, warpx.DistributionMap(m_lev),
-                         m_convertRZmodes2cartesian);
+    if (m_mf_src != nullptr){


The function get_pointer_current_fp should throw instead of having to check the return value in user code.

In order to address this issue, would it be okay to restructure this part of WarpX.H

[[nodiscard]] amrex::MultiFab * get_pointer_Efield_aux (int lev, int direction) const { return Efield_aux[lev][direction].get(); } [[nodiscard]] amrex::MultiFab * get_pointer_Bfield_aux (int lev, int direction) const { return Bfield_aux[lev][direction].get(); } [[nodiscard]] amrex::MultiFab * get_pointer_Efield_fp (int lev, int direction) const { return Efield_fp[lev][direction].get(); } [[nodiscard]] amrex::MultiFab * get_pointer_Bfield_fp (int lev, int direction) const { return Bfield_fp[lev][direction].get(); } [[nodiscard]] amrex::MultiFab * get_pointer_current_fp (int lev, int direction) const { return current_fp[lev][direction].get(); } [[nodiscard]] amrex::MultiFab * get_pointer_current_fp_nodal (int lev, int direction) const { return current_fp_nodal[lev][direction].get(); } [[nodiscard]] amrex::MultiFab * get_pointer_rho_fp (int lev) const { return rho_fp[lev].get(); } [[nodiscard]] amrex::MultiFab * get_pointer_F_fp (int lev) const { return F_fp[lev].get(); } [[nodiscard]] amrex::MultiFab * get_pointer_G_fp (int lev) const { return G_fp[lev].get(); } [[nodiscard]] amrex::MultiFab * get_pointer_phi_fp (int lev) const { return phi_fp[lev].get(); } [[nodiscard]] amrex::MultiFab * get_pointer_vector_potential_fp (int lev, int direction) const { return vector_potential_fp_nodal[lev][direction].get(); } [[nodiscard]] amrex::MultiFab * get_pointer_Efield_cp (int lev, int direction) const { return Efield_cp[lev][direction].get(); } [[nodiscard]] amrex::MultiFab * get_pointer_Bfield_cp (int lev, int direction) const { return Bfield_cp[lev][direction].get(); } [[nodiscard]] amrex::MultiFab * get_pointer_current_cp (int lev, int direction) const { return current_cp[lev][direction].get(); } [[nodiscard]] amrex::MultiFab * get_pointer_rho_cp (int lev) const { return rho_cp[lev].get(); } [[nodiscard]] amrex::MultiFab * get_pointer_F_cp (int lev) const { return F_cp[lev].get(); } [[nodiscard]] amrex::MultiFab * get_pointer_G_cp (int lev) const { return G_cp[lev].get(); } [[nodiscard]] amrex::MultiFab * get_pointer_edge_lengths (int lev, int direction) const { return m_edge_lengths[lev][direction].get(); } [[nodiscard]] amrex::MultiFab * get_pointer_face_areas (int lev, int direction) const { return m_face_areas[lev][direction].get(); }

with something like:

enum class FieldType { Efield_aux, Bfield_aux // ... }; template <FieldType Type> [[nodiscard]] amrex::MultiFab * get_pointer (int lev, int direction) { if (auto* ptr = m_multifabs[lev][direction][Type].get(); ptr != nullptr) return ptr; else WARPX_ABORT_WITH_MESSAGE("..."); }

?

Yes, maybe we can call it get_mf_pointer or get_field_pointer?

Not sure the FieldType must be a template - this is not used in hot loops so it can be a regular argument, no?

Source/Diagnostics/FullDiagnostics.cpp

ax3l · 2024-02-13T00:11:40Z

Source/Particles/PhysicalParticleContainer.cpp

@@ -1202,7 +1202,7 @@ PhysicalParticleContainer::AddPlasma (PlasmaInjector const& plasma_injector, int
        const bool rz_random_theta = m_rz_random_theta;
 #endif
        amrex::ParallelForRNG(overlap_box,
-        [=] AMREX_GPU_DEVICE (int i, int j, int k, amrex::RandomEngine const& engine) noexcept
+        [=] AMREX_GPU_DEVICE (int i, int j, int k, amrex::RandomEngine const& engine) noexcept //NOLINT(clang-analyzer-optin.performance.Padding)


Does this belong in AMReX or at least an AMReX issue instead?

I think that it is something that can happen with any lambda function (e.g., you can have a look here: https://godbolt.org/z/fK6PhvdsG). I suppose it depends on the order in which the variables captured by the lambda function are actually used.

Let's open this as an issue in AMReX, could be interesting for @WeiqunZhang.

Source/ablastr/utils/SignalHandling.cpp

Source/ablastr/parallelization/MPIInitHelpers.H

Source/Particles/PhysicalParticleContainer.cpp

Source/Parallelization/WarpXComm_K.H

Source/Diagnostics/ComputeDiagFunctors/CellCenterFunctor.cpp

Source/Diagnostics/ComputeDiagFunctors/BackTransformFunctor.cpp

WeiqunZhang · 2024-02-13T03:55:25Z

Maybe adding a throw to the end of the assert function could help.

…

On Mon, Feb 12, 2024, 6:57 PM Luca Fedeli ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In Source/Diagnostics/ComputeDiagFunctors/BackTransformFunctor.cpp <#4684 (comment)>: > @@ -60,13 +60,20 @@ BackTransformFunctor::operator ()(amrex::MultiFab& mf_dst, int /*dcomp*/, const const int scomp = 0; // Generate slice of the cell-centered multifab containing boosted-frame field-data // at current z-boost location for the ith buffer - slice = amrex::get_slice_data(moving_window_dir, - m_current_z_boost[i_buffer], - *m_mf_src, - geom, - scomp, - m_mf_src->nComp(), - interpolate); + if (m_mf_src != nullptr){ Same as above. Unfortunately using WARPX_ALWAYS_ASSERT_WITH_MESSAGE does not work here.... — Reply to this email directly, view it on GitHub <#4684 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB37TYOPCD5XQGZH2AODC73YTLJBPAVCNFSM6AAAAABDBDO5BWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQNZWG44TGNRTGM> . You are receiving this because your review was requested.Message ID: ***@***.***>

lucafedeli88 · 2024-02-14T01:37:23Z

WARPX_ALWAYS_ASSERT_WITH_MESSAGE

Very indirectly there is an assert: WARPX_ALWAYS_ASSERT_WITH_MESSAGE calls ablastr::utils::TextMsg::Assert which calls amrex::Assert which finally calls assert. However, ablastr::utils::TextMsg::Assert is in a .cpp file, so the compiler may not be able to figure that out. Should I move it to the corresponding .H file and force inlining?

WeiqunZhang · 2024-02-14T01:46:16Z

I don't think it will help, especially when MPI is on, we call MPI_Abort eventually.

…lang_analyzer

for more information, see https://pre-commit.ci

Source/WarpX.cpp

…lang_analyzer

ax3l · 2024-03-04T11:56:43Z

@lucafedeli88 when you rebase against the latest development, your new AMReX assume should be available now. There is also a small merge conflict right now.

lucafedeli88 · 2024-03-04T16:55:30Z

@lucafedeli88 when you rebase against the latest development, your new AMReX assume should be available now. There is also a small merge conflict right now.

@ax3l , I should have fixed the merge conflict

lucafedeli88 · 2024-03-12T09:34:39Z

ping, @ax3l :-) !

…lang_analyzer

ax3l · 2024-04-01T21:05:58Z

Source/WarpX.H


 enum struct PatchType : int
 {
    fine,
    coarse
 };

+enum struct FieldType : int


This is ok for now, but we should make it a runtime construct so we can add more multifabs later on at runtime and also easily name them.

lucafedeli88 added component: tests Tests and CI cleaning Clean code, improve readability labels Feb 9, 2024

lucafedeli88 added 6 commits February 9, 2024 10:01

Clang tidy CI test: add several clang-analyzer-* checks to clang tidy…

572aa13

… CI test

address issues found with clang-tidy

6ec7402

fix merge conflict

2f5e009

fix bug

ad4c27c

fix spaces

5d8fe0f

simplify

d56840a

lucafedeli88 changed the title ~~[WIP] Clang tidy CI test: add several clang-analyzer-* checks to clang tidy CI test~~ Clang tidy CI test: add several clang-analyzer-* checks to clang tidy CI test Feb 9, 2024

ax3l self-requested a review February 13, 2024 00:05

ax3l self-assigned this Feb 13, 2024