llvm-project icon indicating copy to clipboard operation
llvm-project copied to clipboard

[Clang][analyzer] add documentation for optin performance padding (padding checker) #73675

Open komalverma04 opened this issue 1 year ago • 3 comments

Added documentation for optin.performance.Padding.

  • Performance package has PaddingChecker checker.
  • It checks for excessively padded structs.
    • It has one option that is AllowedPad, an integer option.
    • It's default value is 24 bytes.
      • Reports are generated when padding exceeds AllowedPad.

Purpose

The purpose of this pull request is to improve the clarity and completeness of the documentation for PaddingChecker in the optin.performance.Padding checker.

Closes #73675

komalverma04 avatar Mar 23 '24 21:03 komalverma04

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

github-actions[bot] avatar Mar 23 '24 21:03 github-actions[bot]

@llvm/pr-subscribers-clang-static-analyzer-1

@llvm/pr-subscribers-clang

Author: None (komalverma04)

Changes

Added documentation for optin.performance.Padding.

  • Performance package has PaddingChecker checker.
  • It checks for excessively padded structs.
    • It has one option that is AllowedPad, an integer option.
    • It's default value is 24 bytes.
      • Reports are generated when padding exceeds AllowedPad.

Purpose

The purpose of this pull request is to improve the clarity and completeness of the documentation for PaddingChecker in the optin.performance.Padding checker.

Closes #73675


Full diff: https://github.com/llvm/llvm-project/pull/86411.diff

2 Files Affected:

  • (modified) clang/docs/analyzer/checkers.rst (+20-2)
  • (modified) clang/include/clang/StaticAnalyzer/Checkers/Checkers.td (+1-1)
diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index fe211514914272..64b09bc6ecd1d8 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -804,10 +804,28 @@ Check for performance anti-patterns when using Grand Central Dispatch.
 
 .. _optin-performance-Padding:
 
-optin.performance.Padding
-"""""""""""""""""""""""""
+optin.performance.Padding (PaddingChecker)
+"""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""""
 Check for excessively padded structs.
 
+.. code-block:: objc
+
+ struct TestStruct {
+      int data1;   // 4 bytes
+      char data2;  // 1 byte
+      char padding[27];  // 27 bytes of padding
+    };  // Total size: 32 bytes 
+  
+  void TestFunction() {
+      struct TestStruct struct1;  // Warning is generated due to excessive padding.
+    }
+
+   // Reports are only generated if the excessive padding exceeds 'AllowedPad' in bytes.
+
+  // AllowedPad: Default Value: 24 bytes
+
+   Usage: `AllowedPad=<value>`
+
 .. _optin-portability-UnixAPI:
 
 optin.portability.UnixAPI
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index 686e5e99f4a62c..c0e4e9a70c2ef3 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -908,7 +908,7 @@ def PaddingChecker : Checker<"Padding">,
                   "24",
                   Released>
   ]>,
-  Documentation<NotDocumented>;
+  Documentation<HasDocumentation>;
 
 } // end: "padding"
 

llvmbot avatar Mar 23 '24 21:03 llvmbot

⚠️ We detected that you are using a GitHub private e-mail address to contribute to the repo. Please turn off Keep my email addresses private setting in your account. See LLVM Discourse for more information.

github-actions[bot] avatar Mar 23 '24 21:03 github-actions[bot]

@AtariDreams Thank you for pointing out. I apologise for that mistake. Please review it, if there are more changes.

komalverma04 avatar Mar 24 '24 18:03 komalverma04

I figured, it might be easier to directly propose and apply some of my thoughts. Let me know if you like it.

My goal was:

  • be specific what is the impact of not having the optimal layout: memory, cachemisses
  • have a single block of examples, demonstrating the bad, the good and an alternative good (bitpacking) case, while have no header dependencies
  • rework the example to work with the default configuration of the checker (this makes it more similar to the average user's diagnostics)
  • move all the advanced explanations to the end, as most users won't consider setting a custom threshold
  • make the definition of "when it warns" more accurate. I've checked the code and it seems that the difference counts compared to the optimal layout.
  • discourage the use of pragma pack
  • add guide for fixing the issue, along with a practical hint.

Given that it looks now as I would like it, I'll invite @NagyDonat for a round of review. Let me know if you like it too, or have suggestions for making it better.

Lastly, now the generated html would look like this:

image

steakhal avatar Mar 27 '24 10:03 steakhal

Okay, i completely agree with you. Thank you so much for helping.

komalverma04 avatar Mar 27 '24 12:03 komalverma04

@komalverma04 Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may recieve a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

github-actions[bot] avatar Mar 27 '24 12:03 github-actions[bot]