[Ltrace-devel] [RFC 4/5] Maintain a 1-to-1 mapping between func params and arguments

[Petr Machata]

edgar.iglesias at gmail.com writes:

> From: "Edgar E. Iglesias" <edgar at axis.com>
>
> When mapping function parameters into argument value dictionaries,
> make sure to keep a 1-to-1 mapping between indexes.
>
> For STOP parameters, we insert a ARGTYPE_NONE value to fill out
> the argument value dictionary.
>
> Mainting correct is important because the various param expressions
> refer to eachother based on indexes.

I had a chance to take a look today.  Your fix breaks arg* references in
configuration files, because + becomes a numbered parameter:

$ ./ltrace -a0 -efunc -F <(echo 'int func(int, int, array(int, arg2));') ./ble
ble->func(1, 2, [ 1, 2 ]) = 2
+++ exited (status 2) +++
$ ./ltrace -a0 -efunc -F <(echo 'int func(+int, int, array(int, arg2));') ./ble
ble->func(1, 2, [ 1 ]) = 2
+++ exited (status 2) +++

It's unclear whether it's actually a bug (what arg* should actually mean
is not written anywhere), but I find it unintuitive.  In any case,
currently arg* refers to actual arguments, and it should stay so for
backward compatibility.  The premise that there is any correspondence
between formal and actual arguments is wrong anyway: parameter packs can
expand to any number of actual arguments, including zero.

So the right fix seems to be in the reader:

--- a/read_config_file.c
+++ b/read_config_file.c
@@ -1049,8 +1049,9 @@ process_line(char *buf) {
 		}
 
 		int own;
-		struct arg_type_info *type = parse_lens(&str, &extra_param,
-							fun->num_params, &own);
+		struct arg_type_info *type
+			= parse_lens(&str, &extra_param,
+				     fun->num_params - have_stop, &own);
 		if (type == NULL) {
 			report_error(filename, line_no,
 				     "unknown argument type");

Now, this doesn't fully solve the issue either.  If there is another
pack before "format", things will break again.  There are no packs other
than format at the moment, and that is only allowed once, but in
principle, nothing prevents me from creating a C function with an
interface like this:

  void weird(char const *fmt1, ..., char const *fmt2, ...);

(That's illegal C, of course, but I could make a function that behaves
this way.)  In ltrace, this would be:

  void weird(format, format);

Easy.  Except the latter format will look for its format string in arg2,
because currently, format expands to something like:

  int printf(string, __format_pack(argN));

Where N is whatever the reader thinks is number of the string argument,
which is of course number of _formal_ arguments seen so far.  When
ltrace groks named arguments, it could instead become:

  int printf(tmp=string, __format_pack(tmp));

Which would handle "weird" above as well:

  void weird(tmp1=string, __format_pack(tmp1), tmp2=string, __format_pack(tmp2));

Supporting "format(...)" as pack syntax, keeping the behavior of plain,
argument-less "format" for backward compatibility, would allow even
weirder things like:

  void weirder(fmt1=string, fmt2=string, format(fmt1), format(fmt2));

So, that's about enough of pies in the sky for one day ;)

I pushed your test case, the abort-on-unsupported patch, and my fix.  I
won't push the vector growth patch.  vect is supposed to be useful as a
general growable array, and pushback should therefore have O(1)
amortized complexity.

Thanks,
PM