backslashes in quoted symbol names
Jan Beulich
jbeulich@suse.com
Thu Aug 12 07:02:11 GMT 2021
On 11.06.2021 14:30, Nick Clifton wrote:
>> What we need to consider is how we want to deal with things in the
>> middle of a string. What right now can be expressed as "sym\bol"
>> (backslash will be retained) might need to be changed to "sym\\bol".
>
> The documentation currently suggests that backslashes ought to be
> escape characters. In the "Symbol Names" section of the assembler
> documentation it says:
>
> Multibyte characters are supported. To generate a symbol
> name containing multibyte characters enclose it within
> double quotes and use escape codes. cf See Strings.
>
> So this implies that symbol names inside double quotes are treated as
> strings. It turns out however that this is not true, and multibyte
> characters cannot be encoded in this way.
Afaict they can in some places (when read_symbol_name() is used) but
not in others (when get_symbol_name() is used). Personally I'd be in
favor of using the get_symbol_name() approach everywhere, with more
tight restrictions on what may be used in a symbol name. This is
mainly because, unlike for string literals, I think that without a
way to communicate the used encoding there shouldn't be symbol names
which a consumer may mis-interpret (albeit I realize that, according
to me reading of it, the C standard explicitly permits such).
>> And the question we need to answer up front is what treatment a
>> backslash preceding other than another backslash or a double quote
>> should receive. Imo strictly speaking such uses should be
>> documented as reserved, such that we could alter the behavior down
>> the road, e.g. when it turns out necessary to escape other stuff.
>> That's what we may want to be warning about.
>
> Personally I think that we should follow the documentation on this one
> and treat the backslash character as a real escape character, including
> generating an error when the escaped character is not one with a special
> meaning.
>
>
> Yet then anyone using
>> "sym\\bol" now would still observe a silent change, as we'd convert
>> what now results in two backslashes to just one. I guess this might
>> be acceptable if mentioned in NEWS?
>
> Yes - we should do that. Plus we should extend the documentation to
> make it clear that double quote enclosed symbol names are definitely
> treated as strings.
>
> Maybe we should add a warning for PE based targets that \\ is being
> treated as \ ? Those targets are the only ones where I would imagine
> this situation might actually arise.
I've been slowly making progress with this (without limiting the
diagnostic to PE); I'm now at a point where only some odd testsuite
fallout is left (for the extension to the existing test and one other
one), which I'd rather only spend time on looking into if the general
approach taken is deemed acceptable. The present draft patch is
below; two prereq patches (which I think have merit in their own
right) are attached.
The main question really is that of get_symbol_name() and
read_symbol_name() acting quite differently when it comes to quoted
symbols. At least in case read_symbol_name() was to represent the
"canonical" model, I don't feel it to be in scope for me to address
this more fundamental issue, yet I could see this to be viewed as the
only sensible way out of the mess. (I think it wouldn't be overly
much effort to re-implement read_symbol_name() to be backed by
get_symbol_name(), so if that was the route to go, I might at least
make an attempt - so long as the present very limited handling of
escaped characters would be sufficient, which would mean the
elf/syms.s testcase would have to change.)
As to testsuite fallout:
1) s_{nios2,pru}_set() use get_symbol_name() while s_set() uses
read_symbol_name(). I wonder whether I wouldn't better leave the
target specific functions alone and switch .set in the elf/syms.s
testcase to .equ, .eqv, or .equiv (presumably then also allowing
the #notarget: to be dropped from there).
2) {powerpc,rs6000}-ibm-aix*, tic30-coff, and z80-coff apparently
have not yet understood (by me) parsing issues. For ppc these are
on two of the .globl being added to all/quoted-sym-names.s, yet I
can't spot the target overriding the generic processing of the
directive, so I'm puzzled. I didn't look at the others in any
detail.
Jan
gas: rework handling of backslashes in quoted symbol names
Strange effects can result from the present handling, e.g.:
.if 1
"backslash\\":
.endif
yields first (correctly) "missing closing `"'" but then also "invalid
character '\' in mnemonic" and further "end of file inside conditional".
Symbols names ending in \ are in principle not expressable with that
scheme.
Instead of recording whether a backslash was seen, inspect the
subsequent character right away. Only accept \\ (meaning a single
backslash in the resulting symbol name) and \" (meaning an embedded
double quote in the resulting symbol name) for now, warning about any
other combination.
While perhaps not necessary immediately, also permit concatenated
strings to form a symbol name. This may become useful if going forward
we would want to support \<octal> or \x<hex> sequences, where closing
and re-opening quotes can be useful to delimit such sequences.
---
I'm actually wondering whether the storing of 0 is really necessary when
we did _not_ find a symbol name.
But perhaps a more fundamental question is whether altering the input
buffer is okay; the earlier ia64 patch was primarily added because of
this (albeit I think it has its own merit). After all besides
get_symbol_name() there also is read_symbol_name(), a comment of which
says that it allocates a buffer because of escape character handling. I
have no idea why there are two functions for apparently the same purpose
in the first place, and I question whether quoted symbol names and
operands to e.g. .ascii should really be dealt with by the same
underlying string handling. In any event this is the reason why one of
the .globl directives in the testcase additions needed commenting out,
limiting the usefulness of that part of the test.
--- a/gas/expr.c
+++ b/gas/expr.c
@@ -2395,18 +2395,52 @@ get_symbol_name (char ** ilp_return)
}
else if (c == '"')
{
- bool backslash_seen;
+ char *dst = input_line_pointer;
* ilp_return = input_line_pointer;
- do
+ for (;;)
{
- backslash_seen = c == '\\';
- c = * input_line_pointer ++;
- }
- while (c != 0 && (c != '"' || backslash_seen));
+ c = *input_line_pointer++;
+
+ if (c == 0)
+ {
+ as_warn (_("missing closing '\"'"));
+ break;
+ }
+
+ if (c == '"')
+ {
+ char *ilp_save = input_line_pointer;
+
+ SKIP_WHITESPACE ();
+ if (*input_line_pointer == '"')
+ {
+ ++input_line_pointer;
+ continue;
+ }
+ input_line_pointer = ilp_save;
+ break;
+ }
- if (c == 0)
- as_warn (_("missing closing '\"'"));
+ if (c == '\\')
+ switch (*input_line_pointer)
+ {
+ case '"':
+ case '\\':
+ c = *input_line_pointer++;
+ break;
+
+ default:
+ if (c != 0)
+ as_warn (_("'\\%c' in quoted symbol name; "
+ "behavior may change in the future"),
+ *input_line_pointer);
+ break;
+ }
+
+ *dst++ = c;
+ }
+ *dst = 0;
}
*--input_line_pointer = 0;
return c;
--- a/gas/testsuite/gas/all/quoted-sym-names.d
+++ b/gas/testsuite/gas/all/quoted-sym-names.d
@@ -1,6 +1,9 @@
-#nm: --extern-only
+#nm: --extern-only --numeric-sort
#name: quoted symbol names
#...
0+00 T test-a
-
+0+01 T back\\slash
+0+02 T back"slash
+0+03 T backslash\\
+0+04 T backslash"
--- a/gas/testsuite/gas/all/quoted-sym-names.s
+++ b/gas/testsuite/gas/all/quoted-sym-names.s
@@ -1,4 +1,19 @@
.text
.globl "test-a"
"test-a":
- .word 0
+ .byte 0
+ .globl "back\\slash"
+"back\\slash":
+ .byte 0
+ .globl "back\"slash"
+"back\"slash":
+ .byte 0
+ .globl "backslash\\"
+"backslash\\":
+ .byte 0
+ .globl "backslash\""
+"backslash\"":
+ .byte 0
+# .globl "back""slash"
+"back""slash":
+ .byte 0
-------------- next part --------------
ia64: don't use get_symbol_name() for section parsing
With cross_section() later calling obj_elf_section(), it seems better
to pre-parse the section name by the same function that will be used
there. This way no differences in what is accepted will result.
---
I would have wanted to free the returned buffer, but other users of
obj_elf_section_name() don't free it either, apparently because it may
have got xmalloc()-ed or come from an obstack.
This is a prereq to altering the behavior of get_symbol_name().
--- a/gas/config/tc-ia64.c
+++ b/gas/config/tc-ia64.c
@@ -4773,20 +4773,12 @@ cross_section (int ref, void (*builder)
char *start, *end;
int saved_auto_align;
unsigned int section_count;
- char *name;
- char c;
+ const char *name;
- SKIP_WHITESPACE ();
start = input_line_pointer;
- c = get_symbol_name (&name);
- if (input_line_pointer == start)
- {
- as_bad (_("Missing section name"));
- ignore_rest_of_line ();
- return;
- }
- * input_line_pointer = c;
- SKIP_WHITESPACE_AFTER_NAME ();
+ name = obj_elf_section_name ();
+ if (name == NULL)
+ return;
end = input_line_pointer;
if (*input_line_pointer != ',')
{
-------------- next part --------------
MIPS: don't use get_symbol_name() for section parsing
With s_change_section() later calling obj_elf_section(), it seems better
to pre-parse the section name by the same function that will be used
there. This way no differences in what is accepted will result.
---
I would have wanted to keep freeing the returned buffer, but other users
of obj_elf_section_name() don't free it either, apparently because it
may have got xmalloc()-ed or come from an obstack.
This is a prereq to altering the behavior of get_symbol_name().
--- a/gas/config/tc-mips.c
+++ b/gas/config/tc-mips.c
@@ -16371,33 +16371,29 @@ void
s_change_section (int ignore ATTRIBUTE_UNUSED)
{
char *saved_ilp;
- char *section_name;
- char c, endc;
- char next_c = 0;
+ const char *section_name;
+ char c, next_c = 0;
int section_type;
int section_flag;
int section_entry_size;
int section_alignment;
saved_ilp = input_line_pointer;
- endc = get_symbol_name (§ion_name);
- c = (endc == '"' ? input_line_pointer[1] : endc);
+ section_name = obj_elf_section_name ();
+ if (section_name == NULL)
+ return;
+ c = input_line_pointer[0];
if (c)
- next_c = input_line_pointer [(endc == '"' ? 2 : 1)];
+ next_c = input_line_pointer[1];
/* Do we have .section Name<,"flags">? */
if (c != ',' || (c == ',' && next_c == '"'))
{
- /* Just after name is now '\0'. */
- (void) restore_line_pointer (endc);
input_line_pointer = saved_ilp;
obj_elf_section (ignore);
return;
}
- section_name = xstrdup (section_name);
- c = restore_line_pointer (endc);
-
input_line_pointer++;
/* Do we have .section Name<,type><,flag><,entry_size><,alignment> */
@@ -16442,9 +16438,6 @@ s_change_section (int ignore ATTRIBUTE_U
obj_elf_change_section (section_name, section_type, section_flag,
section_entry_size, 0, 0, 0);
-
- if (now_seg->name != section_name)
- free (section_name);
}
void
More information about the Binutils
mailing list