ffrankies updated this revision to Diff 290396.
ffrankies added a comment.
Rebased, changed import in `StructPackAlignCheck.h` from `../ClangTidy.h` to
`../ClangTidyCheck.h`
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66564/new/
https://reviews.llvm.org/D66564
Files:
clang-tools-extra/clang-tidy/CMakeLists.txt
clang-tools-extra/clang-tidy/ClangTidyForceLinker.h
clang-tools-extra/clang-tidy/altera/AlteraTidyModule.cpp
clang-tools-extra/clang-tidy/altera/CMakeLists.txt
clang-tools-extra/clang-tidy/altera/StructPackAlignCheck.cpp
clang-tools-extra/clang-tidy/altera/StructPackAlignCheck.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/altera-struct-pack-align.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/docs/clang-tidy/index.rst
clang-tools-extra/test/clang-tidy/checkers/altera-struct-pack-align.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/altera-struct-pack-align.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/altera-struct-pack-align.cpp
@@ -0,0 +1,101 @@
+// RUN: %check_clang_tidy %s altera-struct-pack-align %t -- -header-filter=.*
+
+// Struct needs both alignment and packing
+struct error {
+ char a;
+ double b;
+ char c;
+};
+// CHECK-MESSAGES: :[[@LINE-5]]:8: warning: accessing fields in struct 'error' is inefficient due to padding; only needs 10 bytes but is using 24 bytes [altera-struct-pack-align]
+// CHECK-MESSAGES: :[[@LINE-6]]:8: note: use "__attribute__((packed))" to reduce the amount of padding applied to struct 'error'
+// CHECK-MESSAGES: :[[@LINE-7]]:8: warning: accessing fields in struct 'error' is inefficient due to poor alignment; currently aligned to 8 bytes, but recommended alignment is 16 bytes [altera-struct-pack-align]
+// CHECK-MESSAGES: :[[@LINE-8]]:8: note: use "__attribute__((aligned(16)))" to align struct 'error' to 16 bytes
+// CHECK-FIXES: __attribute__((packed))
+// CHECK-FIXES: __attribute__((aligned(16)));
+
+// Struct is explicitly packed, but needs alignment
+struct error_packed {
+ char a;
+ double b;
+ char c;
+} __attribute__((packed));
+// CHECK-MESSAGES: :[[@LINE-5]]:8: warning: accessing fields in struct 'error_packed' is inefficient due to poor alignment; currently aligned to 1 bytes, but recommended alignment is 16 bytes [altera-struct-pack-align]
+// CHECK-MESSAGES: :[[@LINE-6]]:8: note: use "__attribute__((aligned(16)))" to align struct 'error_packed' to 16 bytes
+// CHECK-FIXES: __attribute__((aligned(16)))
+
+// Struct is properly packed, but needs alignment
+struct align_only {
+ char a;
+ char b;
+ char c;
+ char d;
+ int e;
+ double f;
+};
+// CHECK-MESSAGES: :[[@LINE-8]]:8: warning: accessing fields in struct 'align_only' is inefficient due to poor alignment; currently aligned to 8 bytes, but recommended alignment is 16 bytes [altera-struct-pack-align]
+// CHECK-MESSAGES: :[[@LINE-9]]:8: note: use "__attribute__((aligned(16)))" to align struct 'align_only' to 16 bytes
+// CHECK-FIXES: __attribute__((aligned(16)));
+
+// Struct is perfectly packed but wrongly aligned
+struct bad_align {
+ char a;
+ double b;
+ char c;
+} __attribute__((packed)) __attribute__((aligned(8)));
+// CHECK-MESSAGES: :[[@LINE-5]]:8: warning: accessing fields in struct 'bad_align' is inefficient due to poor alignment; currently aligned to 8 bytes, but recommended alignment is 16 bytes [altera-struct-pack-align]
+// CHECK-MESSAGES: :[[@LINE-6]]:8: note: use "__attribute__((aligned(16)))" to align struct 'bad_align' to 16 bytes
+// CHECK-FIXES: __attribute__((aligned(16)));
+
+struct bad_align2 {
+ char a;
+ double b;
+ char c;
+} __attribute__((packed)) __attribute__((aligned(32)));
+// CHECK-MESSAGES: :[[@LINE-5]]:8: warning: accessing fields in struct 'bad_align2' is inefficient due to poor alignment; currently aligned to 32 bytes, but recommended alignment is 16 bytes [altera-struct-pack-align]
+// CHECK-MESSAGES: :[[@LINE-6]]:8: note: use "__attribute__((aligned(16)))" to align struct 'bad_align2' to 16 bytes
+// CHECK-FIXES: __attribute__((aligned(16)));
+
+struct bad_align3 {
+ char a;
+ double b;
+ char c;
+} __attribute__((packed)) __attribute__((aligned(4)));
+// CHECK-MESSAGES: :[[@LINE-5]]:8: warning: accessing fields in struct 'bad_align3' is inefficient due to poor alignment; currently aligned to 4 bytes, but recommended alignment is 16 bytes [altera-struct-pack-align]
+// CHECK-MESSAGES: :[[@LINE-6]]:8: note: use "__attribute__((aligned(16)))" to align struct 'bad_align3' to 16 bytes
+// CHECK-FIXES: __attribute__((aligned(16)));
+
+// Struct is both perfectly packed and aligned
+struct success {
+ char a;
+ double b;
+ char c;
+} __attribute__((packed)) __attribute__((aligned(16)));
+//Should take 10 bytes and be aligned to 16 bytes
+
+// Struct is properly packed, and explicitly aligned
+struct success2 {
+ int a;
+ int b;
+ int c;
+} __attribute__((aligned(16)));
+
+// If struct is properly aligned, packing not needed
+struct success3 {
+ char a;
+ double b;
+ char c;
+} __attribute__((aligned(16)));
+
+// If struct is templated, warnings should not be triggered
+template <typename A, typename B>
+struct success4 {
+ A a;
+ B b;
+ int c;
+};
+
+// Warnings should not trigger on struct instantiations
+void no_trigger_on_instantiation() {
+ struct bad_align3 instantiated { 'a', 0.001, 'b' };
+}
+
Index: clang-tools-extra/docs/clang-tidy/index.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/index.rst
+++ clang-tools-extra/docs/clang-tidy/index.rst
@@ -58,6 +58,7 @@
Name prefix Description
====================== =========================================================
``abseil-`` Checks related to Abseil library.
+``altera-`` Checks related to OpenCL programming for FPGAs.
``android-`` Checks related to Android.
``boost-`` Checks related to Boost library.
``bugprone-`` Checks that target bugprone code constructs.
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -30,6 +30,7 @@
`abseil-time-comparison <abseil-time-comparison.html>`_, "Yes"
`abseil-time-subtraction <abseil-time-subtraction.html>`_, "Yes"
`abseil-upgrade-duration-conversions <abseil-upgrade-duration-conversions.html>`_, "Yes"
+ `altera-struct-pack-align <altera-struct-pack-align.html>`_,
`android-cloexec-accept <android-cloexec-accept.html>`_, "Yes"
`android-cloexec-accept4 <android-cloexec-accept4.html>`_,
`android-cloexec-creat <android-cloexec-creat.html>`_, "Yes"
Index: clang-tools-extra/docs/clang-tidy/checks/altera-struct-pack-align.rst
===================================================================
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/altera-struct-pack-align.rst
@@ -0,0 +1,54 @@
+.. title:: clang-tidy - altera-struct-pack-align
+
+altera-struct-pack-align
+========================
+
+Finds structs that are inefficiently packed or aligned, and recommends
+packing and/or aligning of said structs as needed.
+
+Structs that are not packed take up more space than they should, and accessing
+structs that are not well aligned is inefficient.
+
+Fix-its are provided to fix both of these issues by inserting and/or amending
+relevant struct attributes.
+
+Based on the `Altera SDK for OpenCL: Best Practices Guide
+<https://www.altera.com/en_US/pdfs/literature/hb/opencl-sdk/aocl_optimization_guide.pdf>`_.
+
+.. code-block:: c++
+
+ // The following struct is originally aligned to 4 bytes, and thus takes up
+ // 12 bytes of memory instead of 10. Packing the struct will make it use
+ // only 10 bytes of memory, and aligning it to 16 bytes will make it
+ // efficient to access.
+ struct example {
+ char a; // 1 byte
+ double b; // 8 bytes
+ char c; // 1 byte
+ };
+
+ // The following struct is arranged in such a way that packing is not needed.
+ // However, it is aligned to 4 bytes instead of 8, and thus needs to be
+ // explicitly aligned.
+ struct implicitly_packed_example {
+ char a; // 1 byte
+ char b; // 1 byte
+ char c; // 1 byte
+ char d; // 1 byte
+ int e; // 4 bytes
+ };
+
+ // The following struct is explicitly aligned and packed.
+ struct good_example {
+ char a; // 1 byte
+ double b; // 8 bytes
+ char c; // 1 byte
+ } __attribute__((packed)) __attribute__((aligned(16));
+
+ // Explicitly aligning a struct to the wrong value will result in a warning.
+ // The following example should be aligned to 16 bytes, not 32.
+ struct badly_aligned_example {
+ char a; // 1 byte
+ double b; // 8 bytes
+ char c; // 1 byte
+ } __attribute__((packed)) __attribute__((aligned(32)));
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -67,6 +67,27 @@
Improvements to clang-tidy
--------------------------
+New modules
+^^^^^^^^^^^
+
+- New :doc:`altera <clang-tidy/modules/altera>` module.
+
+ Includes checks related to OpenCL for FPGA coding guidelines, based on the
+ `Altera SDK for OpenCL: Best Practices Guide
+ <https://www.altera.com/en_US/pdfs/literature/hb/opencl-sdk/aocl_optimization_guide.pdf>`_.
+
+New checks
+^^^^^^^^^^
+
+- New :doc:`altera-struct-pack-align
+ <clang-tidy/checks/altera-struct-pack-align>` check.
+
+ Finds structs that are inefficiently packed or aligned, and recommends
+ packing and/or aligning of said structs as needed.
+
+- New :doc:`bugprone-misplaced-pointer-arithmetic-in-alloc
+ <clang-tidy/checks/bugprone-misplaced-pointer-arithmetic-in-alloc>` check.
+
- New :doc:`bugprone-redundant-branch-condition
<clang-tidy/checks/bugprone-redundant-branch-condition>` check.
Index: clang-tools-extra/clang-tidy/altera/StructPackAlignCheck.h
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/altera/StructPackAlignCheck.h
@@ -0,0 +1,41 @@
+//===--- StructPackAlignCheck.h - clang-tidy --------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ALTERA_STRUCTPACKALIGNCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ALTERA_STRUCTPACKALIGNCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace altera {
+
+/// Finds structs that are inefficiently packed or aligned, and recommends
+/// packing and/or aligning of said structs as needed.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/altera-struct-pack-align.html
+class StructPackAlignCheck : public ClangTidyCheck {
+public:
+ StructPackAlignCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context),
+ MaxConfiguredAlignment(Options.get("MaxConfiguredAlignment", 128)) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+ void storeOptions(ClangTidyOptions::OptionMap &Opts);
+
+private:
+ const unsigned MaxConfiguredAlignment;
+ CharUnits computeRecommendedAlignment(CharUnits MinByteSize);
+};
+
+} // namespace altera
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ALTERA_STRUCTPACKALIGNCHECK_H
Index: clang-tools-extra/clang-tidy/altera/StructPackAlignCheck.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/altera/StructPackAlignCheck.cpp
@@ -0,0 +1,144 @@
+//===--- StructPackAlignCheck.cpp - clang-tidy ----------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "StructPackAlignCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/RecordLayout.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include <math.h>
+#include <sstream>
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace altera {
+
+void StructPackAlignCheck::registerMatchers(MatchFinder *Finder) {
+ Finder->addMatcher(recordDecl(isStruct(), isDefinition(),
+ unless(isExpansionInSystemHeader()))
+ .bind("struct"),
+ this);
+}
+
+CharUnits
+StructPackAlignCheck::computeRecommendedAlignment(CharUnits MinByteSize) {
+ CharUnits NewAlign = CharUnits::fromQuantity(1);
+ if (!MinByteSize.isPowerOfTwo()) {
+ int MSB = (int)MinByteSize.getQuantity();
+ for (; MSB > 0; MSB /= 2) {
+ NewAlign = NewAlign.alignTo(
+ CharUnits::fromQuantity(((int)NewAlign.getQuantity()) * 2));
+ // Abort if the computed alignment meets the maximum configured alignment.
+ if (NewAlign.getQuantity() >= MaxConfiguredAlignment)
+ break;
+ }
+ } else {
+ NewAlign = MinByteSize;
+ }
+ return NewAlign;
+}
+
+void StructPackAlignCheck::check(const MatchFinder::MatchResult &Result) {
+ const auto *Struct = Result.Nodes.getNodeAs<RecordDecl>("struct");
+
+ // Do not trigger on templated struct declarations because the packing and
+ // alignment requirements are unknown.
+ if (Struct->isTemplated())
+ return;
+
+ // Get sizing info for the struct.
+ llvm::SmallVector<std::pair<unsigned int, unsigned int>, 10> FieldSizes;
+ unsigned int TotalBitSize = 0;
+ for (const FieldDecl *StructField : Struct->fields()) {
+ // For each StructField, record how big it is (in bits).
+ // Would be good to use a pair of <offset, size> to advise a better
+ // packing order.
+ unsigned int StructFieldWidth =
+ (unsigned int)Result.Context
+ ->getTypeInfo(StructField->getType().getTypePtr())
+ .Width;
+ FieldSizes.emplace_back(StructFieldWidth, StructField->getFieldIndex());
+ // FIXME: Recommend a reorganization of the struct (sort by StructField
+ // size, largest to smallest).
+ TotalBitSize += StructFieldWidth;
+ }
+
+ uint64_t CharSize = Result.Context->getCharWidth();
+ CharUnits CurrSize = Result.Context->getASTRecordLayout(Struct).getSize();
+ CharUnits MinByteSize =
+ CharUnits::fromQuantity(ceil((float)TotalBitSize / CharSize));
+ CharUnits MaxAlign = CharUnits::fromQuantity(
+ ceil((float)Struct->getMaxAlignment() / CharSize));
+ CharUnits CurrAlign =
+ Result.Context->getASTRecordLayout(Struct).getAlignment();
+ CharUnits NewAlign = computeRecommendedAlignment(MinByteSize);
+
+ bool IsPacked = Struct->hasAttr<PackedAttr>();
+ bool NeedsPacking = (MinByteSize < CurrSize) && (MaxAlign != NewAlign) &&
+ (CurrSize != NewAlign);
+ bool NeedsAlignment = CurrAlign.getQuantity() != NewAlign.getQuantity();
+
+ if (!NeedsAlignment && !NeedsPacking)
+ return;
+
+ // If it's using much more space than it needs, suggest packing.
+ // (Do not suggest packing if it is currently explicitly aligned to what the
+ // minimum byte size would suggest as the new alignment.)
+ if (NeedsPacking && !IsPacked) {
+ diag(Struct->getLocation(),
+ "accessing fields in struct %0 is inefficient due to padding; only "
+ "needs %1 bytes but is using %2 bytes")
+ << Struct << (int)MinByteSize.getQuantity()
+ << (int)CurrSize.getQuantity()
+ << FixItHint::CreateInsertion(Struct->getEndLoc().getLocWithOffset(1),
+ " __attribute__((packed))");
+ diag(Struct->getLocation(),
+ "use \"__attribute__((packed))\" to reduce the amount of padding "
+ "applied to struct %0",
+ DiagnosticIDs::Note)
+ << Struct;
+ }
+
+ FixItHint FixIt;
+ AlignedAttr *Attribute = Struct->getAttr<AlignedAttr>();
+ std::string NewAlignQuantity = std::to_string((int)NewAlign.getQuantity());
+ if (Attribute) {
+ std::ostringstream FixItString;
+ FixItString << "aligned(" << NewAlignQuantity << ")";
+ FixIt =
+ FixItHint::CreateReplacement(Attribute->getRange(), FixItString.str());
+ } else {
+ std::ostringstream FixItString;
+ FixItString << " __attribute__((aligned(" << NewAlignQuantity << ")))";
+ FixIt = FixItHint::CreateInsertion(Struct->getEndLoc().getLocWithOffset(1),
+ FixItString.str());
+ }
+
+ // And suggest the minimum power-of-two alignment for the struct as a whole
+ // (with and without packing).
+ if (NeedsAlignment) {
+ diag(Struct->getLocation(),
+ "accessing fields in struct %0 is inefficient due to poor alignment; "
+ "currently aligned to %1 bytes, but recommended alignment is %2 bytes")
+ << Struct << (int)CurrAlign.getQuantity() << NewAlignQuantity << FixIt;
+
+ diag(Struct->getLocation(),
+ "use \"__attribute__((aligned(%0)))\" to align struct %1 to %0 bytes",
+ DiagnosticIDs::Note)
+ << NewAlignQuantity << Struct;
+ }
+}
+
+void StructPackAlignCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+ Options.store(Opts, "MaxConfiguredAlignment", MaxConfiguredAlignment);
+}
+
+} // namespace altera
+} // namespace tidy
+} // namespace clang
Index: clang-tools-extra/clang-tidy/altera/CMakeLists.txt
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/altera/CMakeLists.txt
@@ -0,0 +1,15 @@
+set(LLVM_LINK_COMPONENTS support)
+
+add_clang_library(clangTidyAlteraModule
+ AlteraTidyModule.cpp
+ StructPackAlignCheck.cpp
+
+ LINK_LIBS
+ clangAnalysis
+ clangAST
+ clangASTMatchers
+ clangBasic
+ clangLex
+ clangTidy
+ clangTidyUtils
+ )
Index: clang-tools-extra/clang-tidy/altera/AlteraTidyModule.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/altera/AlteraTidyModule.cpp
@@ -0,0 +1,39 @@
+//===--- AlteraTidyModule.cpp - clang-tidy --------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "../ClangTidy.h"
+#include "../ClangTidyModule.h"
+#include "../ClangTidyModuleRegistry.h"
+#include "StructPackAlignCheck.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace altera {
+
+class AlteraModule : public ClangTidyModule {
+public:
+ void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+ CheckFactories.registerCheck<StructPackAlignCheck>(
+ "altera-struct-pack-align");
+ }
+};
+
+} // namespace altera
+
+// Register the AlteraTidyModule using this statically initialized variable.
+static ClangTidyModuleRegistry::Add<altera::AlteraModule>
+ X("altera-module", "Adds Altera FPGA OpenCL lint checks.");
+
+// This anchor is used to force the linker to link in the generated object file
+// and thus register the AlteraModule.
+volatile int AlteraModuleAnchorSource = 0;
+
+} // namespace tidy
+} // namespace clang
Index: clang-tools-extra/clang-tidy/ClangTidyForceLinker.h
===================================================================
--- clang-tools-extra/clang-tidy/ClangTidyForceLinker.h
+++ clang-tools-extra/clang-tidy/ClangTidyForceLinker.h
@@ -20,6 +20,11 @@
static int LLVM_ATTRIBUTE_UNUSED AbseilModuleAnchorDestination =
AbseilModuleAnchorSource;
+// This anchor is used to force the linker to link the AlteraModule.
+extern volatile int AlteraModuleAnchorSource;
+static int LLVM_ATTRIBUTE_UNUSED AlteraModuleAnchorDestination =
+ AlteraModuleAnchorSource;
+
// This anchor is used to force the linker to link the AndroidModule.
extern volatile int AndroidModuleAnchorSource;
static int LLVM_ATTRIBUTE_UNUSED AndroidModuleAnchorDestination =
Index: clang-tools-extra/clang-tidy/CMakeLists.txt
===================================================================
--- clang-tools-extra/clang-tidy/CMakeLists.txt
+++ clang-tools-extra/clang-tidy/CMakeLists.txt
@@ -46,6 +46,7 @@
# If you add a check, also add it to ClangTidyForceLinker.h in this directory.
add_subdirectory(android)
add_subdirectory(abseil)
+add_subdirectory(altera)
add_subdirectory(boost)
add_subdirectory(bugprone)
add_subdirectory(cert)
@@ -71,6 +72,7 @@
set(ALL_CLANG_TIDY_CHECKS
clangTidyAndroidModule
clangTidyAbseilModule
+ clangTidyAlteraModule
clangTidyBoostModule
clangTidyBugproneModule
clangTidyCERTModule
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
