[PATCH] D66564: [clang-tidy] new altera struct pack align check

[Frank Derry Wanye via Phabricator via cfe-commits]

ffrankies added a comment.

In D66564#2260836 , @aaron.ballman 
wrote:

> I've committed on your behalf in 156b127945a8c923d141e608b7380427da024376 
> . Thank 
> you for the new check!

No problem! Thanks for the commit, and for all the feedback!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66564/new/

https://reviews.llvm.org/D66564

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66564: [clang-tidy] new altera struct pack align check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman closed this revision.
aaron.ballman added a comment.

I've committed on your behalf in 156b127945a8c923d141e608b7380427da024376 
. Thank 
you for the new check!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66564/new/

https://reviews.llvm.org/D66564

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66564: [clang-tidy] new altera struct pack align check

[Frank Derry Wanye via Phabricator via cfe-commits]

ffrankies added a comment.

In D66564#2257635 , @aaron.ballman 
wrote:

> In D66564#2256482 , @ffrankies wrote:
>
>> In D66564#2256424 , @Eugene.Zelenko 
>> wrote:
>>
>>> In D66564#2256423 , @ffrankies 
>>> wrote:
>>>
 @Eugene.Zelenko I don't have commit access to the repository, could you 
 please commit this check on our behalf?
>>>
>>> Sorry, I don't have it either. @aaron.ballman or @njames93 could do this 
>>> for you.
>>
>> No problem. @aaron.ballman @njames93 Could one of you please commit this 
>> check on our behalf?
>
> Sorry for the hassle, but could you rebase again? I'm getting errors when I 
> try to apply the patch.

Done. After rebasing, fixing the include statement in `StructPackAlignCheck.h` 
got rid of the errors that I saw.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66564/new/

https://reviews.llvm.org/D66564

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66564: [clang-tidy] new altera struct pack align check

[Frank Derry Wanye via Phabricator via cfe-commits]

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;
+} 

[PATCH] D66564: [clang-tidy] new altera struct pack align check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D66564#2256482 , @ffrankies wrote:

> In D66564#2256424 , @Eugene.Zelenko 
> wrote:
>
>> In D66564#2256423 , @ffrankies 
>> wrote:
>>
>>> @Eugene.Zelenko I don't have commit access to the repository, could you 
>>> please commit this check on our behalf?
>>
>> Sorry, I don't have it either. @aaron.ballman or @njames93 could do this for 
>> you.
>
> No problem. @aaron.ballman @njames93 Could one of you please commit this 
> check on our behalf?

Sorry for the hassle, but could you rebase again? I'm getting errors when I try 
to apply the patch.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66564/new/

https://reviews.llvm.org/D66564

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66564: [clang-tidy] new altera struct pack align check

[Frank Derry Wanye via Phabricator via cfe-commits]

ffrankies added a comment.

In D66564#2256424 , @Eugene.Zelenko 
wrote:

> In D66564#2256423 , @ffrankies wrote:
>
>> @Eugene.Zelenko I don't have commit access to the repository, could you 
>> please commit this check on our behalf?
>
> Sorry, I don't have it either. @aaron.ballman or @njames93 could do this for 
> you.

No problem. @aaron.ballman @njames93 Could one of you please commit this check 
on our behalf?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66564/new/

https://reviews.llvm.org/D66564

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66564: [clang-tidy] new altera struct pack align check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

In D66564#2256423 , @ffrankies wrote:

> @Eugene.Zelenko I don't have commit access to the repository, could you 
> please commit this check on our behalf?

Sorry, I don't have it either. @aaron.ballman or @njames93 could do this for 
you.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66564/new/

https://reviews.llvm.org/D66564

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66564: [clang-tidy] new altera struct pack align check

[Frank Derry Wanye via Phabricator via cfe-commits]

ffrankies added a comment.

@Eugene.Zelenko I don't have commit access to the repository, could you please 
commit this check on our behalf?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66564/new/

https://reviews.llvm.org/D66564

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66564: [clang-tidy] new altera struct pack align check

[Frank Derry Wanye via Phabricator via cfe-commits]

ffrankies added a comment.

@njames93 Thanks for the clarification! Your suggestion worked, but then I 
realized that I was working off of an improperly worded comment, which I've 
corrected. I looked through the AST Matcher reference, and didn't find anything 
that would trigger on a templated struct declaration. If you know of one I 
missed, please let me know!

If not, could you or someone else commit this patch? I do not have write 
access. My github username is ffrankies , the 
original code for the check was written by psath . If 
the lab itself (vtsynergy ) could be credited, 
that would be awesome.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66564/new/

https://reviews.llvm.org/D66564

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66564: [clang-tidy] new altera struct pack align check

[Frank Derry Wanye via Phabricator via cfe-commits]

ffrankies updated this revision to Diff 283106.
ffrankies added a comment.

Clarified a comment: we don't want the warnings to trigger on templated struct 
//declarations//, not instantiations. We don't want it to trigger on any 
instantiations.

Added a test case that checks if warnings are triggered on instantiation of a 
badly aligned struct.


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)));

[PATCH] D66564: [clang-tidy] new altera struct pack align check

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

In D66564#2185335 , @ffrankies wrote:

> Is there a way around this? And if not, do we ignore it? Lastly, do I need to 
> do anything else since this check has been accepted? There is a "Close 
> Revision" action in the comments, but I'm not sure if that will push the 
> changes or not.

Once its all ready to land and you're happy it can be committed, If you don't 
have commit access, ask one of us to commit on your behalf, We would need to 
know your GitHub name and email mark you as the author of the patch.




Comment at: clang-tools-extra/clang-tidy/altera/StructPackAlignCheck.cpp:50-53
+  // Do not trigger on template instantiations because the packing and
+  // alignment requirements are unknown.
+  if (Struct->isTemplated())
+return;

ffrankies wrote:
> aaron.ballman wrote:
> > I think this should be hoisted into the AST matcher using 
> > `unless(isTemplateInstantiation())`.
> I tried to move this into the matcher like this:
> 
> ```
>   Finder->addMatcher(recordDecl(isStruct(), isDefinition(),
> unless(isExpansionInSystemHeader()),
> unless(isTemplateInstantiation()))
>  .bind("struct"),
>  this);
> ```
> 
> but got an error asking for a `TemplateSpecializationKind` to the 
> `isTemplateInstantiation` function. Did some more digging and according to 
> the docs, this matcher is only available for the CXXRecordDecl class, not the 
> generic RecordDecl.
For the `TemplateSpecializationKind`, You're getting that error as 
`isTemplateInstantiation` is a function that's likely been included in, way 
around that issue is using `ast_matchers::isTemplateInstantiation()`.
For the CXXRecordDecl issue, you can dynCast the recordDecl to a cxxRecordDecl.
The whole code should look something like this
`unless(cxxRecordDecl(ast_matchers::isTemplateInstantiation()))`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66564/new/

https://reviews.llvm.org/D66564

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66564: [clang-tidy] new altera struct pack align check

[Frank Derry Wanye via Phabricator via cfe-commits]

ffrankies added a comment.

Posted this as an inline comment, copying here:

I tried to move this into the matcher like this:

  Finder->addMatcher(recordDecl(isStruct(), isDefinition(),
unless(isExpansionInSystemHeader()),
unless(isTemplateInstantiation()))
 .bind("struct"),
 this);

but got an error asking for a TemplateSpecializationKind parameter to the 
isTemplateInstantiation function. Did some more digging and according to the 
docs, this matcher is only available for the CXXRecordDecl class, not the 
generic RecordDecl.

Is there a way around this? And if not, do we ignore it? Lastly, do I need to 
do anything else since this check has been accepted? There is a "Close 
Revision" action in the comments, but I'm not sure if that will push the 
changes or not.




Comment at: clang-tools-extra/clang-tidy/altera/StructPackAlignCheck.cpp:50-53
+  // Do not trigger on template instantiations because the packing and
+  // alignment requirements are unknown.
+  if (Struct->isTemplated())
+return;

aaron.ballman wrote:
> I think this should be hoisted into the AST matcher using 
> `unless(isTemplateInstantiation())`.
I tried to move this into the matcher like this:

```
  Finder->addMatcher(recordDecl(isStruct(), isDefinition(),
unless(isExpansionInSystemHeader()),
unless(isTemplateInstantiation()))
 .bind("struct"),
 this);
```

but got an error asking for a `TemplateSpecializationKind` to the 
`isTemplateInstantiation` function. Did some more digging and according to the 
docs, this matcher is only available for the CXXRecordDecl class, not the 
generic RecordDecl.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66564/new/

https://reviews.llvm.org/D66564

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66564: [clang-tidy] new altera struct pack align check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

Aside from a minor nit with the matcher, this LGTM!




Comment at: clang-tools-extra/clang-tidy/altera/StructPackAlignCheck.cpp:50-53
+  // Do not trigger on template instantiations because the packing and
+  // alignment requirements are unknown.
+  if (Struct->isTemplated())
+return;

I think this should be hoisted into the AST matcher using 
`unless(isTemplateInstantiation())`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66564/new/

https://reviews.llvm.org/D66564



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66564: [clang-tidy] new altera struct pack align check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

In D66564#2049664 , @ffrankies wrote:

> @Eugene.Zelenko Just checking in, is there anything I missed regarding what 
> we need to do for these checks?


It'll be good idea to ping reviewers.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66564/new/

https://reviews.llvm.org/D66564



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66564: [clang-tidy] new altera struct pack align check

[Frank Derry Wanye via Phabricator via cfe-commits]

ffrankies added a comment.

@Eugene.Zelenko Just checking in, is there anything I missed regarding what we 
need to do for these checks?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66564/new/

https://reviews.llvm.org/D66564



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66564: [clang-tidy] new altera struct pack align check

[Frank Derry Wanye via Phabricator via cfe-commits]

ffrankies updated this revision to Diff 254277.
ffrankies added a comment.

Addressed comments by @aaron.ballman

You're right, we don't want this to trigger on template instantiations and 
structs declared in system headers.

- Check no longer triggers on template instantiations
- Check no longer triggers on structs declared in system headers

To add to @Eugene.Zelenko's comment: there are 5 patches for the `altera` 
module including this one (D72235 , D72218 
, D72241 , 
and D70094 ). We also have 2 checks that have 
been put on the backlog due to time/funding constraints, but could be 
resurrected if that changes.


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,96 @@
+// 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 

[PATCH] D66564: [clang-tidy] new altera struct pack align check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D66564#1895980 , @Eugene.Zelenko 
wrote:

> In D66564#1895916 , @aaron.ballman 
> wrote:
>
> > Are you aware of plans that you or others have for adding additional checks 
> > to the new `altera` module?
>
>
> There are several patches for `altera` module already.


Excellent, thank you for the confirmation!




Comment at: clang-tidy/altera/StructPackAlignCheck.cpp:23
+void StructPackAlignCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(recordDecl(isStruct(), isDefinition()).bind("struct"),
+ this);

I don't think we want this check to trigger on template instantiations because 
the packing and alignment requirements may be different there for each 
instantiation type. e.g.,
```
template 
struct S {
  Ty One;
  int Two;
  Uy Three;
};
```



Comment at: clang-tidy/altera/StructPackAlignCheck.cpp:87
+  if (NeedsPacking && !IsPacked) {
+diag(Struct->getLocation(),
+ "accessing fields in struct %0 is inefficient due to padding; only "

Should this be diagnosing structures declared in system headers? My intuition 
is that we don't want to diagnose those because they're outside of the user's 
control. This can be handled when registering the matchers for the check, if we 
want to ignore those (`unless(isExpansionInSystemHeader())`).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66564/new/

https://reviews.llvm.org/D66564



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66564: [clang-tidy] new altera struct pack align check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

In D66564#1895916 , @aaron.ballman 
wrote:

> Are you aware of plans that you or others have for adding additional checks 
> to the new `altera` module?


There are several patches for `altera` module already.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66564/new/

https://reviews.llvm.org/D66564



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66564: [clang-tidy] new altera struct pack align check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Are you aware of plans that you or others have for adding additional checks to 
the new `altera` module?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66564/new/

https://reviews.llvm.org/D66564



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66564: [clang-tidy] new altera struct pack align check

[Frank Derry Wanye via Phabricator via cfe-commits]

ffrankies updated this revision to Diff 246871.
ffrankies marked 5 inline comments as done.
ffrankies added a comment.

Addressed comments by @aaron.ballman

- Removed commented-out code and irrelevant FIXMEs
- Added Fix-Its to insert/amend the aligned and packed struct attributes as 
needed.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66564/new/

https://reviews.llvm.org/D66564

Files:
  clang-tidy/CMakeLists.txt
  clang-tidy/ClangTidyForceLinker.h
  clang-tidy/altera/AlteraTidyModule.cpp
  clang-tidy/altera/CMakeLists.txt
  clang-tidy/altera/StructPackAlignCheck.cpp
  clang-tidy/altera/StructPackAlignCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/altera-struct-pack-align.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/index.rst
  test/clang-tidy/checkers/altera-struct-pack-align.cpp

Index: test/clang-tidy/checkers/altera-struct-pack-align.cpp
===
--- /dev/null
+++ test/clang-tidy/checkers/altera-struct-pack-align.cpp
@@ -0,0 +1,87 @@
+// RUN: %check_clang_tidy %s altera-struct-pack-align %t -- -header-filter=.* "--" --include opencl-c.h -cl-std=CL1.2 -c
+
+// 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

[PATCH] D66564: [clang-tidy] new altera struct pack align check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/altera/StructPackAlignCheck.cpp:66-68
+  // FIXME: Express this as CharUnit rather than a hardcoded 8-bits (Rshift3)i
+  // After computing the minimum size in bits, check for an existing alignment
+  // flag.

I'm not certain I understand this FIXME, is it out of date? You're getting the 
width of a char and using that rather than hard-coding already.



Comment at: clang-tidy/altera/StructPackAlignCheck.cpp:71
+  CharUnits MinByteSize =
+   //   CharUnits::fromQuantity(ceil((float)TotalBitSize / CHAR_BIT));
+  CharUnits::fromQuantity(ceil((float)TotalBitSize / CharSize));

This should probably be removed.



Comment at: clang-tidy/altera/StructPackAlignCheck.cpp:85
+  ((Struct->getMaxAlignment() / CharSize) != NewAlign.getQuantity()) &&
+//  ((Struct->getMaxAlignment() / CHAR_BIT) != NewAlign.getQuantity()) &&
+  (CurrSize != NewAlign) && !IsPacked) {

This should also probably be removed.



Comment at: clang-tidy/altera/StructPackAlignCheck.cpp:93-94
+diag(Struct->getLocation(),
+ "use \"__attribute__((packed))\" to reduce the amount of padding "
+ "applied to struct %0", DiagnosticIDs::Note)
+<< Struct;

Would it make sense to generate a fixit to add the attribute to the struct so 
long as the struct is not in a system header?



Comment at: clang-tidy/altera/StructPackAlignCheck.cpp:107
+diag(Struct->getLocation(),
+ "use \"__attribute__((aligned(%0)))\" to align struct %1 to %0 bytes",
+ DiagnosticIDs::Note)

Similar question here about the fixit.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66564/new/

https://reviews.llvm.org/D66564



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66564: [clang-tidy] new altera struct pack align check

[Frank Derry Wanye via Phabricator via cfe-commits]

ffrankies updated this revision to Diff 230828.
ffrankies edited the summary of this revision.
ffrankies added a comment.

Implemented changes requested by @aaron.ballman

- Moved the check from the "performance" module into the new "altera" module
- Split the diagnostic message into a warning and a note
- Readability/general code refactoring changes


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66564/new/

https://reviews.llvm.org/D66564

Files:
  clang-tidy/CMakeLists.txt
  clang-tidy/ClangTidyForceLinker.h
  clang-tidy/altera/AlteraTidyModule.cpp
  clang-tidy/altera/CMakeLists.txt
  clang-tidy/altera/StructPackAlignCheck.cpp
  clang-tidy/altera/StructPackAlignCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/altera-struct-pack-align.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/index.rst
  test/clang-tidy/checkers/altera-struct-pack-align.cpp

Index: test/clang-tidy/checkers/altera-struct-pack-align.cpp
===
--- /dev/null
+++ test/clang-tidy/checkers/altera-struct-pack-align.cpp
@@ -0,0 +1,80 @@
+// RUN: %check_clang_tidy %s altera-struct-pack-align %t -- -header-filter=.* "--" --include opencl-c.h -cl-std=CL1.2 -c
+
+// 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
+
+// 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
+
+// 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
+
+// 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
+
+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
+
+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
+
+// 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 error_aligned {
+  char a;
+  double b;
+  char c;
+} __attribute((aligned(16)));
Index: docs/clang-tidy/index.rst
===
--- docs/clang-tidy/index.rst
+++ docs/clang-tidy/index.rst
@@ -59,6 +59,7 @@