On Sun, Oct 14, 2012 at 12:31:04AM +0200, Petr Machata wrote: > [email protected] writes: > > > From: "Edgar E. Iglesias" <[email protected]> > > > > 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.
Great, thanks alot for looking at this! Cheers Edgar _______________________________________________ Ltrace-devel mailing list [email protected] http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel
