[PATCH] D58811: [clang] Fix misuses of char width to char align

[Aleksandr Malykh via Phabricator via cfe-commits]

amalykh created this revision.
amalykh added reviewers: rsmith, momchil.velikov, stephenkelly.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This patch replaces getCharWidth with getCharAlign where it is used incorrectly


Repository:
  rC Clang

https://reviews.llvm.org/D58811

Files:
  clang/include/clang/AST/ASTContext.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/Sema/SemaChecking.cpp


Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -5831,9 +5831,9 @@
   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::max())
   return Diag(TheCall->getBeginLoc(), diag::err_alignment_too_big)
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -1540,7 +1540,7 @@
 }
 
 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()) {
@@ -1594,7 +1594,7 @@
   }
   Align = std::max(Align, getPreferredTypeAlign(T.getTypePtr()));
   if (BaseT.getQualifiers().hasUnaligned())
-Align = Target->getCharWidth();
+Align = Target->getCharAlign();
   if (const auto *VD = dyn_cast(D)) {
 if (VD->hasGlobalStorage() && !ForAlignof)
   Align = std::max(Align, getTargetInfo().getMinGlobalAlign());
Index: clang/include/clang/AST/ASTContext.h
===
--- clang/include/clang/AST/ASTContext.h
+++ clang/include/clang/AST/ASTContext.h
@@ -2103,6 +2103,11 @@
   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.
   ///


Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -5831,9 +5831,9 @@
   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::max())
   return Diag(TheCall->getBeginLoc(), diag::err_alignment_too_big)
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -1540,7 +1540,7 @@
 }
 
 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()) {
@@ -1594,7 +1594,7 @@
   }
   Align = std::max(Align, getPreferredTypeAlign(T.getTypePtr()));
   if (BaseT.getQualifiers().hasUnaligned())
-Align = Target->getCharWidth();
+Align = Target->getCharAlign();
   if (const auto *VD = dyn_cast(D)) {
 if (VD->hasGlobalStorage() && !ForAlignof)
   Align = std::max(Align, getTargetInfo().getMinGlobalAlign());
Index: clang/include/clang/AST/ASTContext.h
===
--- clang/include/clang/AST/ASTContext.h
+++ clang/include/clang/AST/ASTContext.h
@@ -2103,6 +2103,11 @@
   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.
   ///
___
cfe-commits mailing list
cfe-commi

[PATCH] D58811: [clang] Fix misuses of char width to char align

[Aleksandr Malykh via Phabricator via cfe-commits]

amalykh added a comment.

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


Repository:
  rC Clang

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

https://reviews.llvm.org/D58811



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

[PATCH] D58811: [clang] Fix misuses of char width to char align

[Aleksandr Malykh via Phabricator via cfe-commits]

amalykh added a comment.

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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58811



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

[PATCH] D58811: [clang] Fix misuses of char width to char align

[Aleksandr Malykh via Phabricator via cfe-commits]

amalykh added a comment.



> 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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58811



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