Re: [Xen-devel] [XEN PATCH v3 22/23] xen, symbols: rework file symbols selection

[Jan Beulich <jbeulich@suse.com>]

On 26.02.2020 12:33, Anthony PERARD wrote:
> Rework symbols so it prefer file symbols that names an object file to
> file symbols that have a directory component.

I'm afraid I don't understand the distinction you apparently mean to
make: Something having a directory component may still name an
object file. I guess you want to refer to source file names.

> But have symbols still prefer the first file symbol if one of the above
> is true, or prefer the second file symbols if it names a source file
> without directory component.

"one of the above is true" meaning "'object file' or 'has directory
component'"? The first paragraph being a preference statement imo
doesn't lend itself to continuing like this.

Further I guess you mean "last" instead of "second"?

In total I understand the intended order of preference is
- object file name
- source file name with path component(s)
- source file name without any path component

> In a future patch, we are going want to run $(CC) from the root directory
> (xen.git/xen that is). So the guest_walk_%.o files are going to have
> two file symbols, one with a directory component and another one
> which name an object file.

Depending on the Kconfig settings, even today there may be two
file symbols there. Please could you (a) consider both build
modes in you description and (b) make clear - perhaps by way of
giving an example - what would result without your change, and
what will result with in in place (and then also before and
after that future change)? And, knowing they behave differently,
perhaps (c) also cover gcc vs clang (which will then likely also
cover the "why is this" part of the description).

> We still want to prefer the file symbols
> that names an object file, no mater if it is first or second.
> 
> And before running everything from the root directory, we will be able
> to use the same runes to build the guest_%.o as to build any other %.o
> files from %.c files (the rule with the objcopy --redefine-sym).

And when running everything from the root directory, we again
won't be able to? If so, what's the point of mentioning this,
when the almost immediate goal is to run everything from the
root directory?

> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
> --- a/xen/tools/symbols.c
> +++ b/xen/tools/symbols.c
> @@ -80,11 +80,17 @@ static inline int is_arm_mapping_symbol(const char *str)
>  	       && (str[2] == '\0' || str[2] == '.');
>  }
>  
> +enum symbol_type {
> +     symbol = 0,
> +     single_source = 1,
> +     dir_source = 2,
> +     obj_source = 3,

If numeric values matter, please say so in a comment. There's
no need at all to assign numeric values like you do here -
the same numbering will result with the "= <N>" dropped. I
guess you also mean obj_file rather than the pretty ambiguous
obj_source. Similarly with you renaming multi_source to
dir_source, I don't think single_source makes sense anymore.
Maybe simple_source or file_source, and maybe also path_source
instead of dir_source?

> +};
>  static int read_symbol(FILE *in, struct sym_entry *s)

Please have a blank line between these. I don't, however, see
why the scope of this enum gets widened to the entire file.

>  {
>  	char str[500], type[20] = "";
>  	char *sym, stype;
> -	static enum { symbol, single_source, multi_source } last;
> +	static enum symbol_type last;
>  	static char *filename;
>  	int rc = -1;
>  
> @@ -125,13 +131,19 @@ static int read_symbol(FILE *in, struct sym_entry *s)
>  		 * prefer the first one if that names an object file or has a
>  		 * directory component (to cover multiply compiled files).
>  		 */
> -		bool multi = strchr(str, '/') || (sym && sym[1] == 'o');
> -
> -		if (multi || last != multi_source) {
> +		enum symbol_type current;
> +		if (sym && sym[1] == 'o')

Blank line between declaration(s) and statement(s) please.

Jan

> +		    current = obj_source;
> +		else if (strchr(str, '/'))
> +		    current = dir_source;
> +		else
> +		    current = single_source;
> +
> +		if (current > last || last == single_source) {
>  			free(filename);
>  			filename = *str ? strdup(str) : NULL;
> +			last = current;
>  		}
> -		last = multi ? multi_source : single_source;
>  		goto skip_tail;
>  	}
>  
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel