Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clang][bytecode] Don't memcpy() FixedPoint values #123599

Merged
merged 1 commit into from
Jan 20, 2025

Conversation

tbaederr
Copy link
Contributor

llvm::FixedPoint is not trivially copyable.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jan 20, 2025
@llvmbot
Copy link
Member

llvmbot commented Jan 20, 2025

@llvm/pr-subscribers-clang

Author: Timm Baeder (tbaederr)

Changes

llvm::FixedPoint is not trivially copyable.


Full diff: https://github.com/llvm/llvm-project/pull/123599.diff

4 Files Affected:

  • (modified) clang/lib/AST/ByteCode/ByteCodeEmitter.cpp (+6)
  • (modified) clang/lib/AST/ByteCode/Disasm.cpp (+6)
  • (modified) clang/lib/AST/ByteCode/FixedPoint.h (+25)
  • (modified) clang/lib/AST/ByteCode/Interp.h (+7)
diff --git a/clang/lib/AST/ByteCode/ByteCodeEmitter.cpp b/clang/lib/AST/ByteCode/ByteCodeEmitter.cpp
index 3f2bc46664a4ae..19e2416c4c9422 100644
--- a/clang/lib/AST/ByteCode/ByteCodeEmitter.cpp
+++ b/clang/lib/AST/ByteCode/ByteCodeEmitter.cpp
@@ -332,6 +332,12 @@ void emit(Program &P, std::vector<std::byte> &Code, const IntegralAP<true> &Val,
   emitSerialized(Code, Val, Success);
 }
 
+template <>
+void emit(Program &P, std::vector<std::byte> &Code, const FixedPoint &Val,
+          bool &Success) {
+  emitSerialized(Code, Val, Success);
+}
+
 template <typename... Tys>
 bool ByteCodeEmitter::emitOp(Opcode Op, const Tys &...Args,
                              const SourceInfo &SI) {
diff --git a/clang/lib/AST/ByteCode/Disasm.cpp b/clang/lib/AST/ByteCode/Disasm.cpp
index 1aba778eaf7b90..3c55c884a3507c 100644
--- a/clang/lib/AST/ByteCode/Disasm.cpp
+++ b/clang/lib/AST/ByteCode/Disasm.cpp
@@ -62,6 +62,12 @@ inline IntegralAP<true> ReadArg<IntegralAP<true>>(Program &P, CodePtr &OpPC) {
   return I;
 }
 
+template <> inline FixedPoint ReadArg<FixedPoint>(Program &P, CodePtr &OpPC) {
+  FixedPoint I = FixedPoint::deserialize(*OpPC);
+  OpPC += align(I.bytesToSerialize());
+  return I;
+}
+
 LLVM_DUMP_METHOD void Function::dump() const { dump(llvm::errs()); }
 
 LLVM_DUMP_METHOD void Function::dump(llvm::raw_ostream &OS) const {
diff --git a/clang/lib/AST/ByteCode/FixedPoint.h b/clang/lib/AST/ByteCode/FixedPoint.h
index ab8d6d7f02b52f..a4680cbaf0eb41 100644
--- a/clang/lib/AST/ByteCode/FixedPoint.h
+++ b/clang/lib/AST/ByteCode/FixedPoint.h
@@ -91,6 +91,31 @@ class FixedPoint final {
     return ComparisonCategoryResult::Greater;
   }
 
+  size_t bytesToSerialize() const {
+    return sizeof(uint32_t) + (V.getValue().getBitWidth() / CHAR_BIT);
+  }
+
+  void serialize(std::byte *Buff) const {
+    // Semantics followed by APInt.
+    uint32_t SemI = V.getSemantics().toOpaqueInt();
+    std::memcpy(Buff, &SemI, sizeof(SemI));
+
+    llvm::APInt API = V.getValue();
+    llvm::StoreIntToMemory(API, (uint8_t *)(Buff + sizeof(SemI)),
+                           bitWidth() / 8);
+  }
+
+  static FixedPoint deserialize(const std::byte *Buff) {
+    auto Sem = llvm::FixedPointSemantics::getFromOpaqueInt(
+        *reinterpret_cast<const uint32_t *>(Buff));
+    unsigned BitWidth = Sem.getWidth();
+    APInt I(BitWidth, 0ull, !Sem.isSigned());
+    llvm::LoadIntFromMemory(I, (const uint8_t *)Buff + sizeof(uint32_t),
+                            BitWidth / CHAR_BIT);
+
+    return FixedPoint(I, Sem);
+  }
+
   static bool neg(const FixedPoint &A, FixedPoint *R) {
     bool Overflow = false;
     *R = FixedPoint(A.V.negate(&Overflow));
diff --git a/clang/lib/AST/ByteCode/Interp.h b/clang/lib/AST/ByteCode/Interp.h
index 58c0256c7d7df8..510ff88e07f938 100644
--- a/clang/lib/AST/ByteCode/Interp.h
+++ b/clang/lib/AST/ByteCode/Interp.h
@@ -3070,6 +3070,13 @@ inline IntegralAP<true> ReadArg<IntegralAP<true>>(InterpState &S,
   return I;
 }
 
+template <>
+inline FixedPoint ReadArg<FixedPoint>(InterpState &S, CodePtr &OpPC) {
+  FixedPoint FP = FixedPoint::deserialize(*OpPC);
+  OpPC += align(FP.bytesToSerialize());
+  return FP;
+}
+
 } // namespace interp
 } // namespace clang
 

Copy link
Contributor

@Endilll Endilll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe worth adding a release note, especially if you can write a test that would fail without this patch?

clang/lib/AST/ByteCode/FixedPoint.h Show resolved Hide resolved
clang/lib/AST/ByteCode/FixedPoint.h Show resolved Hide resolved
llvm::FixedPoint is not trivially copyable.
@tbaederr tbaederr force-pushed the fixedpoint-serialize branch from 569163a to 9a8e5f8 Compare January 20, 2025 12:32
@tbaederr
Copy link
Contributor Author

Maybe worth adding a release note, especially if you can write a test that would fail without this patch?

I generally don't add change log entries for this stuff since it's an experimental, off-by-default feature.

@tbaederr tbaederr merged commit b5c9cba into llvm:main Jan 20, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants