[PATCH] D72932: [ARM] Follow AACPS standard for volatile bit-fields access width

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added a comment.

I think I've spotted a bug in the ABI spec, which you've faithfully implemented here. I don't know of any other compiler which has implemented this ABI change yet, so it's probably worth seeing if we can get the spec fixed.

The intention of the ABI is to avoid conflicting with the C/C++11 spec, which requires access to one "memory location" to not write to any adjacent memory locations. However, the wording of the ABI does not take into account zero-sized bitfields, which are defined in C/C++ to start a new memory location. For example:

  struct foo {
    int a : 8;
    char  : 0;
    int b : 8;
  };

Here, C/C++ says that `a` and `b` are separate memory locations, so it must be possible to read and write them from two threads. However, the ABI does not have the special case for zero-sized bitfields, so requires access to both fields to use 32-bit loads and stores, which overlap.

I've raised a ticket (internal to Arm) to consider changing the ABI to match C/C++ in this case.



================
Comment at: clang/include/clang/Basic/CodeGenOptions.def:396
+/// according to the field declaring type width.
+CODEGENOPT(ForceNoAAPCSBitfieldWidth, 1, 0)
+
----------------
Why is this a negative option, when the one above is positive?


================
Comment at: clang/include/clang/Driver/Options.td:2328
   HelpText<"Follows the AAPCS standard that all volatile bit-field write generates at least one load. (ARM only).">;
+def ForceNoAAPCSBitfieldWidth : Flag<["-"], "fno-AAPCSBitfieldWidth">, Group<m_arm_Features_Group>,
+  Flags<[DriverOption,CC1Option]>,
----------------
Command-line options are in kebab-case, so this should be something like `fno-aapcs-bitfield-width`. This also applies to the `fAAPCSBitfieldLoad` option above, assuming it's not too late to change that.

Please also add a positive version of this option (i.e. `faapcs-bitfield-width`).


================
Comment at: clang/include/clang/Driver/Options.td:2330
+  Flags<[DriverOption,CC1Option]>,
+  HelpText<"Does not follow the AAPCS standard that volatile bit-field width is dictated by the field declarative type. (ARM only).">;
 
----------------
s/Does/Do/

s/declarative/container/


================
Comment at: clang/lib/CodeGen/CGRecordLayout.h:100
+                 unsigned StorageSize, CharUnits StorageOffset,
+                 unsigned VolatileOffset, unsigned VolatileStorageSize,
+                 CharUnits VolatileStorageOffset)
----------------
It doesn't look like this is ever called with different volatile/non-volatile values, so I don't think we need the extra parameters.


================
Comment at: clang/lib/CodeGen/CGRecordLayoutBuilder.cpp:525
+/// When a volatile bit-field is read, and its container does not overlap
+/// with any non-bit-field member, itscontainer must be read exactly once
+/// using the access width appropriate to the type of the container.
----------------
Space in "itscontainer".


================
Comment at: clang/lib/CodeGen/CGRecordLayoutBuilder.cpp:528
+/// When a volatile bit-field is written, and its container does not overlap
+/// with any non-bit-field member, itscontainer must be read exactly once and
+/// written exactly once using the access width appropriate to thetype of the
----------------
Space in "itscontainer".


================
Comment at: clang/lib/CodeGen/CGRecordLayoutBuilder.cpp:529
+/// with any non-bit-field member, itscontainer must be read exactly once and
+/// written exactly once using the access width appropriate to thetype of the
+/// container. The two accesses are not atomic.
----------------
Space in "thetype".


================
Comment at: clang/lib/CodeGen/CGRecordLayoutBuilder.cpp:541
+    llvm::Type *ResLTy = Types.ConvertTypeForMem(Field->getType());
+    // If the register alignment is less than the type width, we can't enforce a
+    // aligned load, bail out
----------------
_register_ alignment?


================
Comment at: clang/lib/CodeGen/CGRecordLayoutBuilder.cpp:542
+    // If the register alignment is less than the type width, we can't enforce a
+    // aligned load, bail out
+    if ((uint64_t)(Context.toBits(Layout.getAlignment())) <
----------------
Comments end in a full stop (multiple times in this function).


================
Comment at: clang/lib/CodeGen/CGRecordLayoutBuilder.cpp:631
   if (Layout.hasOwnVFPtr())
-    Members.push_back(MemberInfo(CharUnits::Zero(), MemberInfo::VFPtr,
-        llvm::FunctionType::get(getIntNType(32), /*isVarArg=*/true)->
-            getPointerTo()->getPointerTo()));
+    Members.push_back(
+        MemberInfo(CharUnits::Zero(), MemberInfo::VFPtr,
----------------
Unnecessary formatting change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72932