Improve libformat_parser FFI

This should remove a use-after-free as well as simplify the FFI
interface.

gcc/rust/ChangeLog:

	* ast/rust-fmt.cc (Pieces::collect): Handle changes to ffi
	interface.
	(Pieces::~Pieces): Remove function definition.
	(Pieces::Pieces): Likewise.
	(Pieces::operator=): Likewise.
	* ast/rust-fmt.h: Include "optional.h".
	(rust_ffi_alloc): New extern "C" function declaration.
	(rust_ffi_dealloc): Likewise.
	(class FFIVec): New class.
	(class FFIOpt): Likewise.
	(RustHamster::RustHamster): New constructor accepting const
	std::string reference.
	(struct FormatSpec): Use FFIOpt.
	(struct PieceSlice): Remove struct.
	(struct RustString): Likewise.
	(struct FormatArgsHandle): Likewise.
	(collect_pieces): Change function signature.
	(clone_pieces): Likewise.
	(destroy_pieces): Remove extern "C" function declaration.
	(Pieces::~Pieces): Remove function declaration.
	(Pieces::operator=): Likewise.
	(Pieces::get_pieces): Handle changes to class fields.
	(Pieces::Pieces): Remove copy and move constructor declarations,
	adjust signature of remaining constructor declaration.
	(Pieces::pieces_vector): Remove member variable.
	(Pieces::handle): Likewise.
	(Pieces::data): Add member variable.
	* expand/rust-macro-builtins-asm.cc (expand_inline_asm_strings):
	Use references to avoid copying.

libgrust/ChangeLog:

	* libformat_parser/src/lib.rs (struct FFIVec): New.
	(trait StringLeakExt): Remove.
	(struct FFIOpt): New.
	(trait IntoFFI): Adjust implementation for Option.
	(struct RustHamster): Add lifetime and adjust conversion to and
	from &str.
	(enum Piece): Adjust definition to handle changes to
	RustHamster.
	(struct Argument): Likewise.
	(struct FormatSpec): Use FFIOpt and RustHamster.
	(enum Position): Use RustHamster.
	(enum Count): Likewise.
	(struct PieceSlice): Replace with...
	(typedef PieceVec): ...this.
	(struct RustString): Remove.
	(struct FormatArgsHandle): Likewise.
	(fn collect_pieces): Adjust signature, greatly simplifying
	implementation.
	(fn clone_pieces): Likewise.
	(fn destroy_pieces): Remove.
	(trait LayoutExt): New.
	(fn rust_ffi_alloc): New.
	(fn rust_ffi_dealloc): New.

Signed-off-by: Owen Avery <[email protected]>

Author: powerboat9

gcc/rust/ast/rust-fmt.cc

@@ -32,41 +32,11 @@ Pieces
 Pieces::collect (const std::string &to_parse, bool append_newline,
 		 ffi::ParseMode parse_mode)
 {
-  auto handle
-    = ffi::collect_pieces (to_parse.c_str (), append_newline, parse_mode);
-
-  // this performs multiple copies, can we avoid them maybe?
-  // TODO: Instead of just creating a vec of, basically, `ffi::Piece`s, we
-  // should transform them into the proper C++ type which we can work with. so
-  // transform all the strings into C++ strings? all the Option<T> into
-  // tl::optional<T>?
-  auto pieces_vector = std::vector<ffi::Piece> (handle.piece_slice.base_ptr,
-						handle.piece_slice.base_ptr
-						  + handle.piece_slice.len);
-
-  return Pieces (handle, std::move (pieces_vector));
-}
-
-Pieces::~Pieces () { ffi::destroy_pieces (handle); }
-
-Pieces::Pieces (const Pieces &other) : pieces_vector (other.pieces_vector)
-{
-  handle = ffi::clone_pieces (other.handle);
+  Pieces ret (to_parse, ffi::FFIVec<ffi::Piece> ());
+  ret.data->second = ffi::collect_pieces (ffi::RustHamster (ret.data->first),
+					  append_newline, parse_mode);
+  return ret;
 }
 
-Pieces &
-Pieces::operator= (const Pieces &other)
-{
-  handle = ffi::clone_pieces (other.handle);
-  pieces_vector = other.pieces_vector;
-
-  return *this;
-}
-
-Pieces::Pieces (Pieces &&other)
-  : pieces_vector (std::move (other.pieces_vector)),
-    handle (clone_pieces (other.handle))
-{}
-
 } // namespace Fmt
 } // namespace Rust

gcc/rust/ast/rust-fmt.h

@@ -20,20 +20,138 @@
 #define RUST_FMT_H
 
 #include "rust-system.h"
-
-// FIXME: How to encode Option?
+#include "optional.h"
 
 namespace Rust {
 namespace Fmt {
 
 namespace ffi {
 
+extern "C" {
+
+unsigned char *rust_ffi_alloc (size_t count, size_t elem_size, size_t align);
+
+void rust_ffi_dealloc (unsigned char *data, size_t count, size_t elem_size,
+		       size_t align);
+
+} // extern "C"
+
+template <typename T> class FFIVec
+{
+  T *data;
+  size_t len;
+  size_t cap;
+
+public:
+  FFIVec () : data ((T *) alignof (T)), len (0), cap (0) {}
+
+  FFIVec (const FFIVec &) = delete;
+  FFIVec &operator= (const FFIVec &) = delete;
+
+  FFIVec (FFIVec &&other) : data (other.data), len (other.len), cap (other.cap)
+  {
+    other.data = (T *) alignof (T);
+    other.len = 0;
+    other.cap = 0;
+  }
+
+  FFIVec &operator= (FFIVec &&other)
+  {
+    this->~FFIVec ();
+    new (this) FFIVec (std::move (other));
+    return *this;
+  }
+
+  ~FFIVec ()
+  {
+    // T can't be zero-sized
+    if (cap)
+      rust_ffi_dealloc ((unsigned char *) data, cap, sizeof (T), alignof (T));
+  }
+
+  size_t size () const { return len; }
+
+  const T &operator[] (size_t idx) const
+  {
+    rust_assert (idx <= len);
+    return data[idx];
+  }
+
+  T *begin () { return data; }
+  const T *begin () const { return data; }
+  T *end () { return data + len; }
+  const T *end () const { return data + len; }
+};
+
+template <typename T> class FFIOpt
+{
+  struct alignas (T) Inner
+  {
+    char data[sizeof (T)];
+  } inner;
+  bool is_some;
+
+public:
+  template <typename U> FFIOpt (U &&val) : is_some (true)
+  {
+    new (inner.data) T (std::forward<U> (val));
+  }
+
+  FFIOpt () : is_some (false) {}
+
+  FFIOpt (const FFIOpt &other) : is_some (other.is_some)
+  {
+    if (is_some)
+      new (inner.data) T (*(const T *) other.inner.data);
+  }
+
+  FFIOpt (FFIOpt &&other) : is_some (other.is_some)
+  {
+    if (is_some)
+      new (inner.data) T (std::move (*(const T *) other.inner.data));
+  }
+
+  ~FFIOpt ()
+  {
+    if (is_some)
+      ((T *) inner.data)->~T ();
+  }
+
+  FFIOpt &operator= (const FFIOpt &other)
+  {
+    this->~FFIOpt ();
+    new (this) FFIOpt (other);
+    return *this;
+  }
+
+  FFIOpt &operator= (FFIOpt &&other)
+  {
+    this->~FFIOpt ();
+    new (this) FFIOpt (std::move (other));
+    return *this;
+  }
+
+  tl::optional<std::reference_wrapper<T>> get_opt ()
+  {
+    return (T *) inner.data;
+  }
+
+  tl::optional<std::reference_wrapper<const T>> get_opt () const
+  {
+    return (const T *) inner.data;
+  }
+};
+
 struct RustHamster
 {
   const char *ptr;
   size_t len;
 
   std::string to_string () const;
+
+  explicit RustHamster (const std::string &str)
+    : ptr (str.data ()), len (str.size ())
+  {}
 };
 
 /// Enum of alignments which are supported.
@@ -166,33 +284,33 @@ struct Count
 struct FormatSpec
 {
   /// Optionally specified character to fill alignment with.
-  const uint32_t *fill;
+  FFIOpt<uint32_t> fill;
   /// Span of the optionally specified fill character.
-  const InnerSpan *fill_span;
+  FFIOpt<InnerSpan> fill_span;
   /// Optionally specified alignment.
   Alignment align;
   /// The `+` or `-` flag.
-  const Sign *sign;
+  FFIOpt<Sign> sign;
   /// The `#` flag.
   bool alternate;
   /// The `0` flag.
   bool zero_pad;
   /// The `x` or `X` flag. (Only for `Debug`.)
-  const DebugHex *debug_hex;
+  FFIOpt<DebugHex> debug_hex;
   /// The integer precision to use.
   Count precision;
   /// The span of the precision formatting flag (for diagnostics).
-  const InnerSpan *precision_span;
+  FFIOpt<InnerSpan> precision_span;
   /// The string width requested for the resulting format.
   Count width;
   /// The span of the width formatting flag (for diagnostics).
-  const InnerSpan *width_span;
+  FFIOpt<InnerSpan> width_span;
   /// The descriptor string representing the name of the format desired for
   /// this argument, this can be empty or any number of characters, although
   /// it is required to be one word.
   RustHamster ty;
   /// The span of the descriptor string (for diagnostics).
-  const InnerSpan *ty_span;
+  FFIOpt<InnerSpan> ty_span;
 };
 
 /// Representation of an argument specification.
@@ -238,26 +356,6 @@ struct Piece
   };
 };
 
-struct PieceSlice
-{
-  const Piece *base_ptr;
-  size_t len;
-  size_t cap;
-};
-
-struct RustString
-{
-  const unsigned char *ptr;
-  size_t len;
-  size_t cap;
-};
-
-struct FormatArgsHandle
-{
-  PieceSlice piece_slice;
-  RustString rust_string;
-};
-
 enum ParseMode
 {
   Format = 0,
@@ -266,12 +364,10 @@ enum ParseMode
 
 extern "C" {
 
-FormatArgsHandle collect_pieces (const char *input, bool append_newline,
-				 ParseMode parse_mode);
+FFIVec<Piece> collect_pieces (RustHamster input, bool append_newline,
+			      ParseMode parse_mode);
 
-FormatArgsHandle clone_pieces (const FormatArgsHandle &);
-
-void destroy_pieces (FormatArgsHandle);
+FFIVec<Piece> clone_pieces (const FFIVec<Piece> &);
 
 } // extern "C"
 
@@ -281,33 +377,20 @@ struct Pieces
 {
   static Pieces collect (const std::string &to_parse, bool append_newline,
 			 ffi::ParseMode parse_mode);
-  ~Pieces ();
-
-  Pieces (const Pieces &other);
-  Pieces &operator= (const Pieces &other);
 
-  Pieces (Pieces &&other);
-
-  const std::vector<ffi::Piece> &get_pieces () const { return pieces_vector; }
-
-  // {
-  //   slice = clone_pieces (&other.slice);
-  //   to_parse = other.to_parse;
-
-  //   return *this;
-  // }
+  const ffi::FFIVec<ffi::Piece> &get_pieces () const { return data->second; }
 
 private:
-  Pieces (ffi::FormatArgsHandle handle, std::vector<ffi::Piece> &&pieces_vector)
-    : pieces_vector (std::move (pieces_vector)), handle (handle)
+  Pieces (std::string str, ffi::FFIVec<ffi::Piece> pieces)
+    : data (
+      std::make_shared<decltype (data)::element_type> (std::move (str),
+						       std::move (pieces)))
   {}
 
-  std::vector<ffi::Piece> pieces_vector;
-
-  // this memory is held for FFI reasons - it needs to be released and cloned
-  // precisely, so try to not access it/modify it if possible. you should
-  // instead work with `pieces_vector`
-  ffi::FormatArgsHandle handle;
+  // makes copying simpler
+  // also, we'd need to keep the parsed string in a shared_ptr anyways
+  // since we store pointers into the parsed string
+  std::shared_ptr<std::pair<std::string, ffi::FFIVec<ffi::Piece>>> data;
 };
 
 } // namespace Fmt

gcc/rust/expand/rust-macro-builtins-asm.cc

@@ -787,12 +787,12 @@ expand_inline_asm_strings (InlineAsmContext inline_asm_ctx)
 
       auto pieces = Fmt::Pieces::collect (template_str.symbol, false,
 					  Fmt::ffi::ParseMode::InlineAsm);
-      auto pieces_vec = pieces.get_pieces ();
+      auto &pieces_vec = pieces.get_pieces ();
 
       std::string transformed_template_str = "";
       for (size_t i = 0; i < pieces_vec.size (); i++)
 	{
-	  auto piece = pieces_vec[i];
+	  auto &piece = pieces_vec[i];
 	  if (piece.tag == Fmt::ffi::Piece::Tag::String)
 	    {
 	      transformed_template_str += piece.string._0.to_string ();