Merge #935

935: frust-cfg: Only allow double quoted values r=philberty a=CohenArthur

Closes #910 

This PR separates the `handle_cfg_option()` function in two, separating the parsing logic from the session logic. The parsing logic is able to be unit tested, and now only allows quoted values.

What remains to be done is to only allow `key` and `value` to be proper rust identifiers. We need to figure out if we'd like to spawn a parser here and parse identifiers, or simply sanitize both strings to make sure they do not contain invalid characters.

Co-authored-by: Arthur Cohen <[email protected]>

Author: bors[bot]

gcc/rust/Make-lang.in

@@ -66,6 +66,7 @@ GRS_OBJS = \
     rust/rust-gcc.o \
     rust/rust-token.o \
     rust/rust-lex.o \
+    rust/rust-cfg-parser.o \
     rust/rust-parse.o \
     rust/rust-ast-full-test.o \
     rust/rust-session-manager.o \

gcc/rust/parse/rust-cfg-parser.cc

@@ -0,0 +1,75 @@
+#include "rust-cfg-parser.h"
+#include "selftest.h"
+
+namespace Rust {
+bool
+parse_cfg_option (const std::string &input, std::string &key,
+		  std::string &value)
+{
+  key.clear ();
+  value.clear ();
+
+  auto equal = input.find ('=');
+
+  // If there is no equal sign, it means there is no value. Clean up the key
+  // and return
+  if (equal == std::string::npos)
+    {
+      key = input;
+
+      // FIXME: Make sure key is a proper identifier
+
+      return true;
+    }
+
+  key = input.substr (0, equal);
+
+  auto remaining_input = input.substr (equal + 1);
+  if (remaining_input[0] != '"' || remaining_input.back () != '"')
+    return false;
+
+  // Remove the quotes around the value, by advancing one character
+  value = remaining_input.substr (1);
+  // And trimming the rightmost character. This is fine since we've already
+  // checked that both the first and last characters were quotes.
+  value.resize (value.size () - 1);
+
+  // FIXME: We need to sanitize here and make sure that both key and value
+  // are proper identifiers
+
+  return true;
+}
+
+} // namespace Rust
+
+#if CHECKING_P
+
+namespace selftest {
+
+void
+rust_cfg_parser_test (void)
+{
+  std::string key;
+  std::string value;
+
+  ASSERT_TRUE (Rust::parse_cfg_option ("key-no-value", key, value));
+  ASSERT_EQ (key, "key-no-value");
+  ASSERT_TRUE (value.empty ());
+
+  ASSERT_TRUE (Rust::parse_cfg_option ("k=\"v\"", key, value));
+  ASSERT_EQ (key, "k");
+  ASSERT_EQ (value, "v");
+
+  // values should be between double quotes
+  ASSERT_FALSE (Rust::parse_cfg_option ("k=v", key, value));
+
+  // No value is an error if there is an equal sign
+  ASSERT_FALSE (Rust::parse_cfg_option ("k=", key, value));
+
+  // No key is an error
+  ASSERT_FALSE (Rust::parse_cfg_option ("=", key, value));
+  ASSERT_FALSE (Rust::parse_cfg_option ("=value", key, value));
+}
+} // namespace selftest
+
+#endif // CHECKING_P

gcc/rust/parse/rust-cfg-parser.h

@@ -0,0 +1,57 @@
+/* This file is part of GCC.
+
+GCC is free software; you can redistribute it and/or modify
+it under the terms of the GNU General Public License as published by
+the Free Software Foundation; either version 3, or (at your option)
+any later version.
+
+GCC is distributed in the hope that it will be useful,
+but WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+GNU General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with GCC; see the file COPYING3.  If not see
+<http://www.gnu.org/licenses/>. */
+
+#ifndef RUST_CFG_PARSER_H
+#define RUST_CFG_PARSER_H
+
+#include "config.h"
+#include "system.h"
+#include "coretypes.h"
+
+#include <string>
+
+namespace Rust {
+/**
+ * Parse a `key` or `key="value"` pair given to the `-frust-cfg` compiler
+ * option.
+ *
+ * The format is as follows:
+ *
+ * -frust-cfg=<input>
+ *
+ * cfg_input: identifier | identifier '=' '"' identifier '"'
+ *
+ * @param input User input given to the -frust-cfg option
+ * @param key String in which to store the parsed `key`.
+ * @param value String in which to store the parsed `value` if it exists
+ *
+ * @return false if the given input was invalid, true otherwise
+ */
+bool
+parse_cfg_option (const std::string &input, std::string &key,
+		  std::string &value);
+} // namespace Rust
+
+#if CHECKING_P
+
+namespace selftest {
+extern void
+rust_cfg_parser_test (void);
+} // namespace selftest
+
+#endif // CHECKING_P
+
+#endif // RUST_CFG_PARSER_H

gcc/rust/parse/rust-parse.cc

@@ -118,5 +118,4 @@ extract_module_path (const AST::AttrVec &inner_attrs,
 
   return path;
 }
-
 } // namespace Rust

gcc/rust/rust-lang.cc

@@ -33,6 +33,7 @@
 #include "langhooks.h"
 #include "langhooks-def.h"
 #include "selftest.h"
+#include "rust-cfg-parser.h"
 
 #include <mpfr.h>
 // note: header files must be in this order or else forward declarations don't
@@ -453,6 +454,7 @@ run_rust_tests ()
 {
   // Call tests for the rust frontend here
   simple_assert ();
+  rust_cfg_parser_test ();
 }
 } // namespace selftest
 

gcc/rust/rust-session-manager.cc

@@ -29,6 +29,7 @@
 #include "rust-tycheck-dump.h"
 #include "rust-ast-resolve-unused.h"
 #include "rust-compile.h"
+#include "rust-cfg-parser.h"
 
 #include "diagnostic.h"
 #include "input.h"
@@ -382,87 +383,22 @@ Session::handle_cfg_option (const std::string &input)
   std::string key;
   std::string value;
 
-  enum pstate
-  {
-    KEY,
-    EQ,
-    VAL,
-    DONE,
-    ERROR
-  };
-
-  // FIXME
-  // we need to use the GCC self_test framework to unit-test this its
-  // likely got a bunch of bugs. This simple parser could be extracted to a
-  // helper function to be more easily unit-tested or it could be tested via
-  // checking what the target_options contain
-  bool expect_quote = false;
-  pstate s = KEY;
-  for (const auto &ch : input)
+  // Refactor this if needed
+  if (!parse_cfg_option (input, key, value))
     {
-      if (ch == ' ')
-	{
-	  if (!key.empty ())
-	    s = EQ;
-	  else if (!value.empty ())
-	    s = DONE;
-	  else
-	    {
-	      s = ERROR;
-	      break;
-	    }
-	}
-      else if (ch == '"')
-	{
-	  expect_quote = !expect_quote;
-	}
-      else if (ch == '=')
-	{
-	  if (key.empty ())
-	    {
-	      s = ERROR;
-	      break;
-	    }
-
-	  s = VAL;
-	}
-      else
-	{
-	  if (s == KEY)
-	    key.push_back (ch);
-	  else if (s == VAL)
-	    value.push_back (ch);
-	  else
-	    {
-	      s = ERROR;
-	      break;
-	    }
-	}
-    }
-
-  if (key.empty () && value.empty ())
-    s = ERROR;
-
-  if (expect_quote)
-    s = ERROR;
-
-  if (s == ERROR)
-    {
-      rust_error_at (Location (),
-		     "invalid %<-frust-cfg=option%> expected %<key%> or "
-		     "key=%<value%> got %<%s%>",
-		     input.c_str ());
+      rust_error_at (
+	Location (),
+	"invalid argument to %<-frust-cfg%>: Accepted formats are "
+	"%<-frust-cfg=key%> or %<-frust-cfg=key=\"value\"%> (quoted)");
       return false;
     }
 
   if (value.empty ())
-    {
-      // rustc does not seem to error on dup key
-      options.target_data.insert_key (key);
-      return true;
-    }
+    // rustc does not seem to error on dup key
+    options.target_data.insert_key (key);
+  else
+    options.target_data.insert_key_value_pair (key, value);
 
-  options.target_data.insert_key_value_pair (key, value);
   return true;
 }
 

gcc/testsuite/rust/compile/cfg5.rs

@@ -1,4 +1,4 @@
-// { dg-additional-options "-w -frust-cfg=A=B" }
+// { dg-additional-options "-w -frust-cfg=A=\"B\"" }
 struct Foo;
 impl Foo {
     #[cfg(A = "B")]