[clang] Fix misuses of char width to char align

[amalykh]

Summary

This patch replaces getCharWidth with getCharAlign where it is used incorrectly

[lebedev.ri]

Can you be more specific?
Is there some bugreport this fixes?
Is there some test?

[amalykh]

Currently getCharWidth and getCharAlign functions are hard coded to return 8 in clang with no way to change it for target.
This can be seen in TargetInfo.h file:

unsigned getCharWidth() const { return 8; } // FIXME
unsigned getCharAlign() const { return 8; } // FIXME

It means that these functions can be used interchangeably and there will be no test failures because of this, until custom alignment will be supported.
Nevertheless, it will not be possible to support custom alignment until all incorrect usages of getCharWidth will be replaced with getCharAlign. This patch fixes some of them

[lebedev.ri]

In D58811#1414784, @amalykh wrote:

Currently getCharWidth and getCharAlign functions are hard coded to return 8 in clang with no way to change it for target.
This can be seen in TargetInfo.h file:

unsigned getCharWidth() const { return 8; } // FIXME
unsigned getCharAlign() const { return 8; } // FIXME

It means that these functions can be used interchangeably and there will be no test failures because of this, until custom alignment will be supported.
Nevertheless, it will not be possible to support custom alignment until all incorrect usages of getCharWidth will be replaced with getCharAlign. This patch fixes some of them

Likewise, LLVM hardcodes that byte is 8 bits and does not want to change that
since there is no official LLVM target with that characteristics,
and thus it is impossible to test it, and it would quickly regress.

Which target will be affected by this patch?
I suspect the situation is the same here.
If there is no way to test this upstream...

[amalykh]

Thank you for your comments, Roman.
Despite the fact, that there are no official LLVM targets that will be influenced by this patch, there are architectures which support only word addressing mode, for which the assumption of char align equals to 8 is not true.
The patch have been tested for a proprietary architecture, where there is a difference between align and width of char. As for the official targets, it's more like a style improvement.

[lebedev.ri]

In D58811#1417959, @amalykh wrote:

Thank you for your comments, Roman.
Despite the fact, that there are no official LLVM targets that will be influenced by this patch, there are architectures which support only word addressing mode, for which the assumption of char align equals to 8 is not true.
The patch have been tested for a proprietary architecture, where there is a difference between align and width of char. As for the official targets, it's more like a style improvement.

Thank you for working on this!

So in other words this will sadly indeed not have any test coverage in LLVM proper, correct?
If so, i think you want to submit an RFC (mentioning this fact) to cfe-dev (maybe CC llvm-dev).

[efriedma]

The patch have been tested for a proprietary architecture, where there is a difference between align and width of char.

The C standard requires that both alignof(char) and sizeof(char) must equal 1, and therefore must equal each other. Could you clarify the characteristics of your target?

[amalykh]

Could you clarify the characteristics of your target?

Yeah, sure.
The target supports only word addressing when it comes to loading from memory or storing to memory, that's why types shorter than 4 bytes need to be aligned by at least 4.
In order to achieve this, getCharAlign and getShortAlign functions and some llvm passes have been modified to work with such kind of layout.

The C standard requires that both alignof(char) and sizeof(char) must equal 1, and therefore must equal each other

Currently we are targeting C99 language standard, which, as far as I know, says nothing about alignof keyword. Anyway, thanks for pointing that out.

[lebedev.ri]

This review seems to be stuck/dead, consider abandoning if no longer relevant.

clang/include/clang/AST/ASTContext.h

Optional<CharUnits> getTypeSizeInCharsIfKnown(const Type *Ty) const {
     return getTypeSizeInCharsIfKnown(QualType(Ty, 0));
   }
 
   /// Return the ABI-specified alignment of a (complete) type \p T, in
   /// bits.
   unsigned getTypeAlign(QualType T) const { return getTypeInfo(T).Align; }
   unsigned getTypeAlign(const Type *T) const { return getTypeInfo(T).Align; }
 
+  /// Return the alignment of the character type, in bits.
+  uint64_t getCharAlign() const {
+    return getTypeAlign(CharTy);
+  }
+
   /// Return the ABI-specified natural alignment of a (complete) type \p T,
   /// before alignment adjustments, in bits.
   ///
   /// This alignment is curently used only by ARM and AArch64 when passing
   /// arguments of a composite type.
   unsigned getTypeUnadjustedAlign(QualType T) const {
     return getTypeUnadjustedAlign(T.getTypePtr());
   }

clang/lib/AST/ASTContext.cpp

const llvm::fltSemantics &ASTContext::getFloatTypeSemantics(QualType T) const {
   case BuiltinType::Float:      return Target->getFloatFormat();
   case BuiltinType::Double:     return Target->getDoubleFormat();
   case BuiltinType::LongDouble: return Target->getLongDoubleFormat();
   case BuiltinType::Float128:   return Target->getFloat128Format();
   }
 }
 
 CharUnits ASTContext::getDeclAlign(const Decl *D, bool ForAlignof) const {
-  unsigned Align = Target->getCharWidth();
+  unsigned Align = Target->getCharAlign();
 
   bool UseAlignAttrOnly = false;
   if (unsigned AlignFromAttr = D->getMaxAlignment()) {
     Align = AlignFromAttr;
 
     // __attribute__((aligned)) can increase or decrease alignment
     // *except* on a struct or struct member, where it only increases
     // alignment unless 'packed' is also specified.
else if (!BaseT->isIncompleteType()) {
             Align = std::max(Align, Target->getLargeArrayAlign());
           else if (isa<ConstantArrayType>(arrayType) &&
                    MinWidth <= getTypeSize(cast<ConstantArrayType>(arrayType)))
             Align = std::max(Align, Target->getLargeArrayAlign());
         }
       }
       Align = std::max(Align, getPreferredTypeAlign(T.getTypePtr()));
       if (BaseT.getQualifiers().hasUnaligned())
-        Align = Target->getCharWidth();
+        Align = Target->getCharAlign();
       if (const auto *VD = dyn_cast<VarDecl>(D)) {
         if (VD->hasGlobalStorage() && !ForAlignof)
           Align = std::max(Align, getTargetInfo().getMinGlobalAlign());
       }
     }
 
     // Fields can be subject to extra alignment constraints, like if
     // the field is packed, the struct is packed, or the struct has a

clang/lib/Sema/SemaChecking.cpp

if (const auto *UE =
             << Arg->getSourceRange();
 
     llvm::APSInt Result = Arg->EvaluateKnownConstInt(Context);
 
     if (!Result.isPowerOf2())
       return Diag(TheCall->getBeginLoc(), diag::err_alignment_not_power_of_two)
              << Arg->getSourceRange();
 
-    if (Result < Context.getCharWidth())
+    if (Result < Context.getCharAlign())
       return Diag(TheCall->getBeginLoc(), diag::err_alignment_too_small)
-             << (unsigned)Context.getCharWidth() << Arg->getSourceRange();
+             << (unsigned)Context.getCharAlign() << Arg->getSourceRange();
 
     if (Result > std::numeric_limits<int32_t>::max())
       return Diag(TheCall->getBeginLoc(), diag::err_alignment_too_big)
              << std::numeric_limits<int32_t>::max() << Arg->getSourceRange();
   }
 
   return false;
 }