Page MenuHomePhabricator

[AArch64][ARM][Clang] Unaligned Access Warning Added
ClosedPublic

Authored by mubashar_ on Dec 23 2021, 8:59 AM.

Details

Summary

Added warning for potential cases of unaligned access when option -mno-unaligned-access has been specified.

Diff Detail

Event Timeline

mubashar_ created this revision.Dec 23 2021, 8:59 AM
mubashar_ requested review of this revision.Dec 23 2021, 8:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 23 2021, 8:59 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
pbarrio requested changes to this revision.Dec 23 2021, 9:48 AM
pbarrio added a subscriber: pbarrio.

Suggest clarifying the commit message to something like:

"Added warning for potential cases of unaligned access when option -mno-unaligned-access has been specified".

Nice testing :)

clang/lib/Driver/ToolChains/Arch/AArch64.cpp
468–469

Nit: preserve coding style of the file. Move the bracket to the previous line, preceded by a space. I.e.

if (A->getOption().matches(options::OPT_mno_unaligned_access)) {

This revision now requires changes to proceed.Dec 23 2021, 9:48 AM
mubashar_ updated this revision to Diff 396151.Dec 24 2021, 3:43 AM
mubashar_ edited the summary of this revision. (Show Details)

Cleaned up code to make it more clang format friendly. Added another test suite in C.

mubashar_ marked an inline comment as done.Dec 24 2021, 4:02 AM
lenary added a comment.Jan 4 2022, 6:51 AM

Two quick comments about when this is enabled, and then I'm happy!

clang/lib/Driver/ToolChains/Arch/ARM.cpp
772–774

I think we should be enabling this warning everywhere we enable +strict-align between lines 772 and 813, even though in some of those cases an explicit argument has not been provided to the compiler to enable or disable unaligned accesses.

This should probably also be the case for aarch64 too!

786–791

I think you want the curly braces here, as the condition on 774 is looking for both -munaligned-access and -mno-unaligned-access, and then line 776 checks which direction the last argument actually went.

mubashar_ updated this revision to Diff 397322.Jan 4 2022, 9:30 AM
mubashar_ marked 2 inline comments as done.

Enabled warning whenever +strict-align is present.

lenary accepted this revision.Jan 4 2022, 9:43 AM

LGTM! Thanks for your hard work on this!

I think you've addressed @pbarrio's comments on this, and I'm not sure when he's back, so wait 48h before landing this, in case there are further comments.

lenary added a comment.Jan 7 2022, 2:50 AM

This caused some driver test failures. They have been fixed in https://reviews.llvm.org/rG11c67e5a4e99f51ec66c9781710f81955cfd5e24

thakis added a subscriber: thakis.Jan 7 2022, 4:12 AM

Thanks for the patch!

  1. When committing code, please add "Differential Revision: https://reviews.llvm.org/D116221" as last line to your commit description, to link the commit to the review. See git log for many examples.
  1. This breaks tests on Windows, see e.g. http://45.33.8.238/win/52066/step_7.txt Please take a look, and revert for now if it takes a while to fix.
lenary added a comment.Jan 7 2022, 5:13 AM

Reverted for the moment in rG3aec4b3d348d69e6c1ead7cba54677b855293983 while we fix the windows tests.

mubashar_ updated this revision to Diff 399271.Jan 12 2022, 2:32 AM
mubashar_ set the repository for this revision to rG LLVM Github Monorepo.

Windows tests are now expected to fail.

This is coming together well.

I think structs U23 to U26 in the C test are duplicates - how about surrounding them all in #pragma pack(push, 2) so they're different from the earlier cases?

Quite a few more comments inline on the tests.

clang/include/clang/Basic/DiagnosticASTKinds.td
596–597

I think the warning text here is misleading, as we are not inspecting the alignment of the parent, we're inspecting the original alignment of the field's struct type vs the resulting alignment of the field.

How about this wording, with %2 standing for the field type's name, which you will have to give to the Diag() formatter.

Unfortunately, this will mean the wording of the expectations in the tests need to be updated.

clang/lib/Driver/ToolChains/Arch/AArch64.cpp
470–473

Please guard all the CmdArgs.push_back(…) with if (!ForAS) please - otherwise you get the assembler warnings that I had to fix last time (the same goes for these in ARM.cpp)

clang/test/Sema/test-wunaligned-access.c
168

isn't this the same case as struct U1 in this file?

174

this looks like the same case as struct U5.

180

This looks the same as struct U8

189

this looks the same as struct U16.

300

This had me scratching my head a bit. Did you expect this to apply to the field itself? Looking at the AST, it doesn't: https://godbolt.org/z/c47KorT3G (note no MaxAlign attribute on the struct)

323–342

These struct definitions are unused.

Matt added a subscriber: Matt.Jan 13 2022, 10:55 AM
mubashar_ updated this revision to Diff 400572.Jan 17 2022, 9:08 AM
mubashar_ marked 8 inline comments as done.

Addressed comments.

mubashar_ updated this revision to Diff 400827.Jan 18 2022, 6:28 AM

Addressed comments from @lenary.

lenary accepted this revision.Jan 20 2022, 2:14 AM

LGTM. I've looked a lot closer at the tests this time around, and am happy with them, and I think the message is phrased better, and there shouldn't be warnings with -cc1as.

pbarrio accepted this revision.Jan 20 2022, 4:44 AM

LGTM too.

This revision is now accepted and ready to land.Jan 20 2022, 4:44 AM
This revision was landed with ongoing or failed builds.Jan 20 2022, 6:13 AM
This revision was automatically updated to reflect the committed changes.
ychen added a subscriber: ychen.Feb 7 2022, 11:50 AM
ychen added inline comments.
clang/lib/Driver/ToolChains/Arch/AArch64.cpp
223–224

Sorry if it has been discussed, why the compiler option is not added during constructing the clang job but here? All kinds of clients could call this method, only clang care about this option.

Allen added a subscriber: Allen.Sat, May 21, 11:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptSat, May 21, 11:08 PM
Herald added a subscriber: MaskRay. · View Herald Transcript