Merge #1417

1417: unsafe: check uses of static variables r=CohenArthur a=CohenArthur

Addresses #1411 

Accessing an extern static or a mut static is unsafe and requires an unsafe function or block

Co-authored-by: Arthur Cohen <arthur.cohen@embecosm.com>

7 files changed, 180 insertions, 4 deletions

diff --git a/gcc/rust/checks/errors/rust-unsafe-checker.cc b/gcc/rust/checks/errors/rust-unsafe-checker.cc
index def3cc1..d234179 100644
--- a/gcc/rust/checks/errors/rust-unsafe-checker.cc
+++ b/gcc/rust/checks/errors/rust-unsafe-checker.cc
@@ -25,7 +25,10 @@
 namespace Rust {
 namespace HIR {
 
-UnsafeChecker::UnsafeChecker () : context (*Resolver::TypeCheckContext::get ())
+UnsafeChecker::UnsafeChecker ()
+  : context (*Resolver::TypeCheckContext::get ()),
+    resolver (*Resolver::Resolver::get ()),
+    mappings (*Analysis::Mappings::get ())
 {}
 
 void
@@ -35,6 +38,47 @@ UnsafeChecker::go (HIR::Crate &crate)
     item->accept_vis (*this);
 }
 
+static void
+check_static_mut (HIR::Item *maybe_static, Location locus)
+{
+  if (maybe_static->get_hir_kind () == Node::BaseKind::VIS_ITEM)
+    {
+      auto item = static_cast<Item *> (maybe_static);
+      if (item->get_item_kind () == Item::ItemKind::Static)
+	{
+	  auto static_item = static_cast<StaticItem *> (item);
+	  if (static_item->is_mut ())
+	    rust_error_at (
+	      locus, "use of mutable static requires unsafe function or block");
+	}
+    }
+}
+
+static void
+check_extern_static (HIR::ExternalItem *maybe_static, Location locus)
+{
+  if (maybe_static->get_extern_kind () == ExternalItem::ExternKind::Static)
+    rust_error_at (locus,
+		   "use of extern static requires unsafe function or block");
+}
+
+void
+UnsafeChecker::check_use_of_static (HirId node_id, Location locus)
+{
+  if (is_unsafe_context ())
+    return;
+
+  auto maybe_static_mut = mappings.lookup_hir_item (node_id);
+  auto maybe_extern_static = mappings.lookup_hir_extern_item (node_id);
+
+  if (maybe_static_mut)
+    check_static_mut (maybe_static_mut, locus);
+
+  if (maybe_extern_static)
+    check_extern_static (static_cast<ExternalItem *> (maybe_extern_static),
+			 locus);
+}
+
 void
 UnsafeChecker::push_unsafe (HirId id)
 {
@@ -60,7 +104,18 @@ UnsafeChecker::is_unsafe_context ()
 
 void
 UnsafeChecker::visit (IdentifierExpr &ident_expr)
-{}
+{
+  NodeId ast_node_id = ident_expr.get_mappings ().get_nodeid ();
+  NodeId ref_node_id;
+  HirId definition_id;
+
+  if (!resolver.lookup_resolved_name (ast_node_id, &ref_node_id))
+    return;
+
+  rust_assert (mappings.lookup_node_to_hir (ref_node_id, &definition_id));
+
+  check_use_of_static (definition_id, ident_expr.get_locus ());
+}
 
 void
 UnsafeChecker::visit (Lifetime &lifetime)
@@ -72,7 +127,18 @@ UnsafeChecker::visit (LifetimeParam &lifetime_param)
 
 void
 UnsafeChecker::visit (PathInExpression &path)
-{}
+{
+  NodeId ast_node_id = path.get_mappings ().get_nodeid ();
+  NodeId ref_node_id;
+  HirId definition_id;
+
+  if (!resolver.lookup_resolved_name (ast_node_id, &ref_node_id))
+    return;
+
+  rust_assert (mappings.lookup_node_to_hir (ref_node_id, &definition_id));
+
+  check_use_of_static (definition_id, path.get_locus ());
+}
 
 void
 UnsafeChecker::visit (TypePathSegment &segment)
@@ -253,7 +319,11 @@ UnsafeChecker::visit (StructExprStructBase &expr)
 
 void
 UnsafeChecker::visit (CallExpr &expr)
-{}
+{
+  if (expr.has_params ())
+    for (auto &arg : expr.get_arguments ())
+      arg->accept_vis (*this);
+}
 
 void
 UnsafeChecker::visit (MethodCallExpr &expr)
diff --git a/gcc/rust/checks/errors/rust-unsafe-checker.h b/gcc/rust/checks/errors/rust-unsafe-checker.h
index 3c81707..38b9019 100644
--- a/gcc/rust/checks/errors/rust-unsafe-checker.h
+++ b/gcc/rust/checks/errors/rust-unsafe-checker.h
@@ -20,6 +20,7 @@
 #define RUST_UNSAFE_CHECKER_H
 
 #include "rust-hir-visitor.h"
+#include "rust-name-resolver.h"
 #include "rust-hir-type-check.h"
 
 namespace Rust {
@@ -51,7 +52,15 @@ private:
    */
   bool is_unsafe_context ();
 
+  /**
+   * Check if a mutable static or external static item is used outside of an
+   * unsafe context
+   */
+  void check_use_of_static (HirId node_id, Location locus);
+
   Resolver::TypeCheckContext &context;
+  Resolver::Resolver resolver;
+  Analysis::Mappings mappings;
 
   virtual void visit (IdentifierExpr &ident_expr) override;
   virtual void visit (Lifetime &lifetime) override;
diff --git a/gcc/rust/hir/tree/rust-hir-item.h b/gcc/rust/hir/tree/rust-hir-item.h
index 0d41bd0..394b04f 100644
--- a/gcc/rust/hir/tree/rust-hir-item.h
+++ b/gcc/rust/hir/tree/rust-hir-item.h
@@ -723,6 +723,8 @@ public:
 
   Location get_locus () const override final { return locus; }
 
+  ItemKind get_item_kind () const override { return ItemKind::Module; }
+
 protected:
   /* Use covariance to implement clone function as returning this object
    * rather than base */
@@ -773,6 +775,8 @@ public:
 
   Location get_locus () const override final { return locus; }
 
+  ItemKind get_item_kind () const override { return ItemKind::ExternCrate; }
+
   void accept_vis (HIRFullVisitor &vis) override;
   void accept_vis (HIRStmtVisitor &vis) override;
   void accept_vis (HIRVisItemVisitor &vis) override;
@@ -1039,6 +1043,7 @@ public:
   UseDeclaration &operator= (UseDeclaration &&other) = default;
 
   Location get_locus () const override final { return locus; }
+  ItemKind get_item_kind () const override { return ItemKind::UseDeclaration; }
 
   void accept_vis (HIRFullVisitor &vis) override;
   void accept_vis (HIRStmtVisitor &vis) override;
@@ -1094,6 +1099,8 @@ public:
     return ImplItem::ImplItemType::FUNCTION;
   }
 
+  ItemKind get_item_kind () const override { return ItemKind::Function; }
+
   // Mega-constructor with all possible fields
   Function (Analysis::NodeMapping mappings, Identifier function_name,
 	    FunctionQualifiers qualifiers,
@@ -1329,6 +1336,8 @@ public:
 
   Identifier get_new_type_name () const { return new_type_name; }
 
+  ItemKind get_item_kind () const override { return ItemKind::TypeAlias; }
+
   Analysis::NodeMapping get_impl_mappings () const override
   {
     return get_mappings ();
@@ -1373,6 +1382,7 @@ public:
   bool has_where_clause () const { return !where_clause.is_empty (); }
 
   Location get_locus () const override final { return locus; }
+  ItemKind get_item_kind () const override { return ItemKind::Struct; }
 
   std::vector<std::unique_ptr<GenericParam>> &get_generic_params ()
   {
@@ -1709,6 +1719,8 @@ public:
 
   Identifier get_identifier () const { return variant_name; }
 
+  ItemKind get_item_kind () const override { return ItemKind::EnumItem; }
+
 protected:
   EnumItem *clone_item_impl () const override { return new EnumItem (*this); }
 };
@@ -1930,6 +1942,7 @@ public:
   void accept_vis (HIRVisItemVisitor &vis) override;
 
   Identifier get_identifier () const { return enum_name; }
+  ItemKind get_item_kind () const override { return ItemKind::Enum; }
 
   std::vector<std::unique_ptr<GenericParam>> &get_generic_params ()
   {
@@ -2037,6 +2050,8 @@ public:
 
   WhereClause &get_where_clause () { return where_clause; }
 
+  ItemKind get_item_kind () const override { return ItemKind::Union; }
+
 protected:
   /* Use covariance to implement clone function as returning this object
    * rather than base */
@@ -2111,6 +2126,8 @@ public:
     return ImplItem::ImplItemType::CONSTANT;
   }
 
+  ItemKind get_item_kind () const override { return ItemKind::Constant; }
+
 protected:
   /* Use covariance to implement clone function as returning this object
    * rather than base */
@@ -2188,6 +2205,8 @@ public:
 
   Type *get_type () { return type.get (); }
 
+  ItemKind get_item_kind () const override { return ItemKind::Static; }
+
 protected:
   StaticItem *clone_item_impl () const override
   {
@@ -2677,6 +2696,8 @@ public:
     return type_param_bounds;
   }
 
+  ItemKind get_item_kind () const override { return ItemKind::Trait; }
+
 protected:
   /* Use covariance to implement clone function as returning this object
    * rather than base */
@@ -2797,6 +2818,8 @@ public:
 
   WhereClause &get_where_clause () { return where_clause; }
 
+  ItemKind get_item_kind () const override { return ItemKind::Impl; }
+
 protected:
   ImplBlock *clone_item_impl () const override { return new ImplBlock (*this); }
 };
@@ -2811,10 +2834,18 @@ class ExternalItem : public Node
   Location locus;
 
 public:
+  enum class ExternKind
+  {
+    Static,
+    Function,
+  };
+
   virtual ~ExternalItem () {}
 
   BaseKind get_hir_kind () override final { return EXTERNAL; }
 
+  virtual ExternKind get_extern_kind () = 0;
+
   // Returns whether item has outer attributes.
   bool has_outer_attrs () const { return !outer_attrs.empty (); }
 
@@ -2921,6 +2952,8 @@ public:
 
   std::unique_ptr<Type> &get_item_type () { return item_type; }
 
+  ExternKind get_extern_kind () override { return ExternKind::Static; }
+
 protected:
   /* Use covariance to implement clone function as returning this object
    * rather than base */
@@ -3072,6 +3105,8 @@ public:
 
   bool is_variadic () const { return has_variadics; }
 
+  ExternKind get_extern_kind () override { return ExternKind::Function; }
+
 protected:
   /* Use covariance to implement clone function as returning this object
    * rather than base */
@@ -3149,6 +3184,8 @@ public:
     return extern_items;
   }
 
+  ItemKind get_item_kind () const override { return ItemKind::ExternBlock; }
+
 protected:
   /* Use covariance to implement clone function as returning this object
    * rather than base */
diff --git a/gcc/rust/hir/tree/rust-hir.h b/gcc/rust/hir/tree/rust-hir.h
index 58f4db2..c2f6fef 100644
--- a/gcc/rust/hir/tree/rust-hir.h
+++ b/gcc/rust/hir/tree/rust-hir.h
@@ -175,6 +175,26 @@ class Item : public Stmt
   // TODO: should outer attrs be defined here or in each derived class?
 
 public:
+  enum class ItemKind
+  {
+    Static,
+    Constant,
+    TypeAlias,
+    Function,
+    UseDeclaration,
+    ExternBlock,
+    ExternCrate,
+    Struct,
+    Union,
+    Enum,
+    EnumItem, // FIXME: ARTHUR: Do we need that?
+    Trait,
+    Impl,
+    Module,
+  };
+
+  virtual ItemKind get_item_kind () const = 0;
+
   // Unique pointer custom clone function
   std::unique_ptr<Item> clone_item () const
   {
diff --git a/gcc/testsuite/rust/compile/unsafe1.rs b/gcc/testsuite/rust/compile/unsafe1.rs
new file mode 100644
index 0000000..9cd3f6b
--- /dev/null
+++ b/gcc/testsuite/rust/compile/unsafe1.rs
@@ -0,0 +1,14 @@
+fn foo(_a: &i32) {}
+fn bar(_a: i32) {}
+
+static mut a: i32 = 15;
+
+fn main() {
+    foo(&a); // { dg-error "use of mutable static" }
+    bar(a); // { dg-error "use of mutable static" }
+
+    unsafe {
+        foo(&a);
+        bar(a);
+    }
+}
diff --git a/gcc/testsuite/rust/compile/unsafe2.rs b/gcc/testsuite/rust/compile/unsafe2.rs
new file mode 100644
index 0000000..e03e4bc
--- /dev/null
+++ b/gcc/testsuite/rust/compile/unsafe2.rs
@@ -0,0 +1,16 @@
+fn foo(_a: &i32) {}
+fn bar(_a: i32) {}
+
+mod inner {
+    pub static mut a: i32 = 15;
+}
+
+fn main() {
+    foo(&inner::a); // { dg-error "use of mutable static" }
+    bar(inner::a); // { dg-error "use of mutable static" }
+
+    unsafe {
+        foo(&inner::a);
+        bar(inner::a);
+    }
+}
diff --git a/gcc/testsuite/rust/compile/unsafe3.rs b/gcc/testsuite/rust/compile/unsafe3.rs
new file mode 100644
index 0000000..56aec76
--- /dev/null
+++ b/gcc/testsuite/rust/compile/unsafe3.rs
@@ -0,0 +1,10 @@
+extern "C" {
+    static VALUE: char;
+}
+
+fn main() {
+    let _ = VALUE; // { dg-error "use of extern static" }
+    unsafe {
+        let _ = VALUE;
+    }
+}