Re: support C/C++ identifiers named with non-ASCII characters

[張俊芝 <zjz@zjz.name>]

[-- Attachment #1: Type: text/plain, Size: 2879 bytes --]

Simon Marchi 於 2018/5/21 下午10:03 寫道:
> On 2018-05-21 04:52 AM, 張俊芝 wrote:
> 
> Hi Zhang,
> 
> Thanks for the patch, I tested it quickly, it seems to work as expected.
> 
> Could you please write a small test case in testsuite/gdb.base with the example
> you gave, so we make sure this doesn't get broken later?  If you can write it
> in such a way that both clang and gcc understand it would be better, because
> most people run the testuite using gcc to compile test programs.
> 
> I am not a specialist in lexing and parsing C, so can you explain quickly why
> you think this is a good solution?  Quickly, I understand that you change the
> identifier recognition algorithm to a blacklist of characters rather than
> a whitelist, so bytes that are not recognized (such as those that compose
> the utf-8 encoded characters) are not rejected.
> 
> Given unlimited time, would the right solution be to use a lib to parse the
> string as utf-8, and reject strings that are not valid utf-8?
> 
> Here are some not and formatting comments:
> 
>> +static bool is_identifier_separator (char);
> 
> You don't have to forward declare the function if it's not necessary.
> 
>> +    /* '<' should not be a token separator, because it can be an open angle
>> +       bracket followed by a nested template identifier in C++. */
> 
> Please use two spaces after the final period (...C++.  */).
> 
>> +  if (is_identifier_separator(c))
> 
> Please use a space before the parentheses:
> 
>    is_identifier_separator (c)
> 
> 
>> +  for (c = tokstart[namelen]; !is_identifier_separator(c);)
> 
> Here too.
> 
> Thanks!
> 
> Simon
> 

Thank you for the reply, Simon.

This new diff addresses all the code style issues you mentioned.

Yes, you are right in that the blacklist is limited. Actually, 
`is_identifier_separator` only blacklists the invalid ASCII characters 
for an identifier, leaving all the invalid non-ASCII characters unchecked.

So seems it would be better if non-ASCII characters were also checked. 
However, unfortunately, GDB is neither aware of the encoding of the 
terminal input, nor the encoding of the generated symbolic information 
in the executable. So the blacklist is made to restrict to the invalid 
ASCII characters in order to support all ASCII-compliant encodings.

Having said that, I find that it does no harm to the user if we only 
check the ASCII characters. If the user is trying to print an identifier 
which includes an invalid non-ASCII character, say, p 測】, where 】is 
invalid, they will get an error message:

No symbol "測】" in current context.

which doesn't seem any worse than an error message like:

Invalid character "】" in expression.

Perhaps the former might be even more intuitional.

So personally, I think it might be safe enough to use this limited 
blacklist method.

[-- Attachment #2: diff --]
[-- Type: text/plain, Size: 2741 bytes --]

diff --git a/gdb/c-exp.y b/gdb/c-exp.y
index 5e10d2a3b4..e10b6b474d 100644
--- a/gdb/c-exp.y
+++ b/gdb/c-exp.y
@@ -1718,6 +1718,53 @@ type_aggregate_p (struct type *type)
 	      && TYPE_DECLARED_CLASS (type)));
 }
 
+/* While iterating all the characters in an identifier, an identifier separator
+   is a boundary where we know the iteration is done. */
+
+static bool
+is_identifier_separator (char c)
+{
+  switch (c)
+    {
+    case ' ':
+    case '\t':
+    case '\n':
+    case '\0':
+    case '\'':
+    case '"':
+    case '\\':
+    case '(':
+    case ')':
+    case ',':
+    case '.':
+    case '+':
+    case '-':
+    case '*':
+    case '/':
+    case '|':
+    case '&':
+    case '^':
+    case '~':
+    case '!':
+    case '@':
+    case '[':
+    case ']':
+    /* '<' should not be a token separator, because it can be an open angle
+       bracket followed by a nested template identifier in C++.  */
+    case '>':
+    case '?':
+    case ':':
+    case '=':
+    case '{':
+    case '}':
+    case ';':
+      return true;
+    default:
+      break;
+    }
+  return false;
+}
+
 /* Validate a parameter typelist.  */
 
 static void
@@ -1920,7 +1967,7 @@ parse_number (struct parser_state *par_state,
 	 FIXME: This check is wrong; for example it doesn't find overflow
 	 on 0x123456789 when LONGEST is 32 bits.  */
       if (c != 'l' && c != 'u' && n != 0)
-	{	
+	{
 	  if ((unsigned_p && (ULONGEST) prevn >= (ULONGEST) n))
 	    error (_("Numeric constant too large."));
 	}
@@ -2741,16 +2788,13 @@ lex_one_token (struct parser_state *par_state, bool *is_quoted_name)
       }
     }
 
-  if (!(c == '_' || c == '$'
-	|| (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z')))
+  if (is_identifier_separator (c))
     /* We must have come across a bad character (e.g. ';').  */
     error (_("Invalid character '%c' in expression."), c);
 
   /* It's a name.  See how long it is.  */
   namelen = 0;
-  for (c = tokstart[namelen];
-       (c == '_' || c == '$' || (c >= '0' && c <= '9')
-	|| (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || c == '<');)
+  for (c = tokstart[namelen]; !is_identifier_separator (c);)
     {
       /* Template parameter lists are part of the name.
 	 FIXME: This mishandles `print $a<4&&$a>3'.  */
@@ -2932,7 +2976,7 @@ classify_name (struct parser_state *par_state, const struct block *block,
 	 filename.  However, if the name was quoted, then it is better
 	 to check for a filename or a block, since this is the only
 	 way the user has of requiring the extension to be used.  */
-      if ((is_a_field_of_this.type == NULL && !is_after_structop) 
+      if ((is_a_field_of_this.type == NULL && !is_after_structop)
 	  || is_quoted_name)
 	{
 	  /* See if it's a file name. */