Re: [PATCH] Improved LINENO support
On Fri, Mar 11, 2011 at 09:56:49PM +, Harald van Dijk wrote: > > Signed-off-by: Harald van Dijk Both patches applied. Thanks a lot! -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe dash" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Improved LINENO support
On Thu, 2011-03-10 at 16:10 +0800, Herbert Xu wrote:
> On Sat, Nov 27, 2010 at 04:56:17PM +, Harald van Dijk wrote:
> >
> > Again, comments are welcome.
>
> Thanks for working on this! Just a few minor problems to correct.
> Oh and please add a changelog + sign-off.
Done.
> Please don't add any new uses of STATIC. I'm fine with leaving
> existing ones in place though.
Changed to static.
> I like this bit :)
Thanks :)
> Please separate out unrelated changes like this into distinct
> patches.
It is not unrelated: I changed the meaning of struct funcnode's field n
to refer to the function definition, rather than the list of the
function's commands, because I needed to refer to the function
definition node from evalfun, which only gets passed a funcnode. But it
is something that could be applied independently (without being useful
by itself), so I've attached it as a separate patch for easier review.
> Please use fmtstr instead of sprintf for consistency.
Done.
Cheers,
Harald
Signed-off-by: Harald van Dijk
diff --git a/ChangeLog b/ChangeLog
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2011-03-11 Harald van Dijk
+
+ * Let funcnode refer to a function definition, not its first command.
+
2011-03-10 Jonathan Nieder
* Dotcmd should exit with zero when doing nothing.
diff --git a/src/eval.c b/src/eval.c
--- a/src/eval.c
+++ b/src/eval.c
@@ -296,7 +296,7 @@ calleval:
}
goto success;
case NDEFUN:
- defun(n->narg.text, n->narg.next);
+ defun(n);
success:
status = 0;
setstatus:
@@ -954,7 +954,7 @@ evalfun(struct funcnode *func, int argc,
shellparam.optind = 1;
shellparam.optoff = -1;
pushlocalvars();
- evaltree(&func->n, flags & EV_TESTED);
+ evaltree(func->n.narg.next, flags & EV_TESTED);
poplocalvars(0);
funcdone:
INTOFF;
diff --git a/src/exec.c b/src/exec.c
--- a/src/exec.c
+++ b/src/exec.c
@@ -688,14 +688,14 @@ addcmdentry(char *name, struct cmdentry
*/
void
-defun(char *name, union node *func)
+defun(union node *func)
{
struct cmdentry entry;
INTOFF;
entry.cmdtype = CMDFUNCTION;
entry.u.func = copyfunc(func);
- addcmdentry(name, &entry);
+ addcmdentry(func->narg.text, &entry);
INTON;
}
diff --git a/src/exec.h b/src/exec.h
--- a/src/exec.h
+++ b/src/exec.h
@@ -71,7 +71,7 @@ void changepath(const char *);
#ifdef notdef
void getcmdentry(char *, struct cmdentry *);
#endif
-void defun(char *, union node *);
+void defun(union node *);
void unsetfunc(const char *);
int typecmd(int, char **);
int commandcmd(int, char **);
Signed-off-by: Harald van Dijk
diff -r 772aea7ed8c5 ChangeLog
--- a/ChangeLog Fri Mar 11 22:39:28 2011 +0100
+++ b/ChangeLog Fri Mar 11 22:45:57 2011 +0100
@@ -1,3 +1,7 @@
+2011-03-11 Harald van Dijk
+
+ * Improve LINENO support.
+
2011-03-11 Harald van Dijk
* Let funcnode refer to a function definition, not its first command.
diff -r 772aea7ed8c5 src/error.c
--- a/src/error.c Fri Mar 11 22:39:28 2011 +0100
+++ b/src/error.c Fri Mar 11 22:45:57 2011 +0100
@@ -62,6 +62,7 @@
int exception;
int suppressint;
volatile sig_atomic_t intpending;
+int errlinno;
static void exverror(int, const char *, va_list)
@@ -116,13 +117,12 @@
const char *fmt;
errs = out2;
- name = arg0 ?: "sh";
- fmt = "%s: ";
- if (commandname) {
- name = commandname;
+ name = arg0 ? arg0 : "sh";
+ if (!commandname)
fmt = "%s: %d: ";
- }
- outfmt(errs, fmt, name, startlinno);
+ else
+ fmt = "%s: %d: %s: ";
+ outfmt(errs, fmt, name, errlinno, commandname);
doformat(errs, msg, ap);
#if FLUSHERR
outc('\n', errs);
diff -r 772aea7ed8c5 src/error.h
--- a/src/error.h Fri Mar 11 22:39:28 2011 +0100
+++ b/src/error.h Fri Mar 11 22:45:57 2011 +0100
@@ -120,6 +120,7 @@
#else
void onint(void);
#endif
+extern int errlinno;
void sh_error(const char *, ...) __attribute__((__noreturn__));
void exerror(int, const char *, ...) __attribute__((__noreturn__));
const char *errmsg(int, int);
diff -r 772aea7ed8c5 src/eval.c
--- a/src/eval.c Fri Mar 11 22:39:28 2011 +0100
+++ b/src/eval.c Fri Mar 11 22:45:57 2011 +0100
@@ -73,7 +73,7 @@
int evalskip; /* set if we are skipping commands */
STATIC int skipcount; /* number of levels to skip */
MKINIT int loopnest; /* current loop nesting level */
-static int funcnest; /* depth of function calls */
+static int funcline; /* starting line number of current function, or 0 if not in a function */
char *commandname;
@@ -218,6 +218,9 @@
status = !exitstatus;
goto setstatus;
case NREDIR:
+ errlinno = lineno = n->nredir.linno;
+ if (funcline)
+ lineno -= funcline - 1;
expredir(n->nredir.redirect);
pushredir(n->nredir.redirect);
status = redirectsafe(n->nredir.redirect, REDIR_PUSH);
@@ -376,6 +379,10 @@
struct strlist *sp;
struct stackmark smark;
+ errlinno = lineno = n->nfor.linno;
+ if (funcline)
+ lineno -= funcline - 1;
+
setstackmark(&smark);
arglist.lastp = &arglist.list;
for (argp = n->nfor.args ; argp ; argp = argp->narg.next) {
@@ -417,6 +424,10 @@
Re: [PATCH] Improved LINENO support
On Sat, Nov 27, 2010 at 04:56:17PM +, Harald van Dijk wrote:
>
> Again, comments are welcome.
Thanks for working on this! Just a few minor problems to correct.
Oh and please add a changelog + sign-off.
> diff --git a/src/eval.c b/src/eval.c
> index b966749..d85f66f 100644
> --- a/src/eval.c
> +++ b/src/eval.c
> @@ -73,7 +73,7 @@
> int evalskip;/* set if we are skipping commands */
> STATIC int skipcount;/* number of levels to skip */
> MKINIT int loopnest; /* current loop nesting level */
> -static int funcnest; /* depth of function calls */
> +STATIC int funcline; /* starting line number of current function, or
> 0 if not in a function */
Please don't add any new uses of STATIC. I'm fine with leaving
existing ones in place though.
> @@ -730,7 +749,7 @@ evalcommand(union node *cmd, int flags)
> *nargv = NULL;
>
> lastarg = NULL;
> - if (iflag && funcnest == 0 && argc > 0)
> + if (iflag && funcline == 0 && argc > 0)
> lastarg = nargv[-1];
I like this bit :)
> diff --git a/src/exec.c b/src/exec.c
> index 42299ea..959b9f4 100644
> --- a/src/exec.c
> +++ b/src/exec.c
> @@ -688,14 +688,14 @@ addcmdentry(char *name, struct cmdentry *entry)
> */
>
> void
> -defun(char *name, union node *func)
> +defun(union node *func)
> {
> struct cmdentry entry;
>
> INTOFF;
> entry.cmdtype = CMDFUNCTION;
> entry.u.func = copyfunc(func);
> - addcmdentry(name, &entry);
> + addcmdentry(func->ndefun.text, &entry);
> INTON;
> }
Please separate out unrelated changes like this into distinct
patches.
> @@ -329,6 +333,9 @@ lookupvar(const char *name)
> struct var *v;
>
> if ((v = *findvar(hashvar(name), name)) && !(v->flags & VUNSET)) {
> + if (v == &vlineno && v->text == linenovar) {
> + sprintf(linenoval(), "%d", lineno);
> + }
Please use fmtstr instead of sprintf for consistency.
Cheers,
--
Email: Herbert Xu
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe dash" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Improved LINENO support
On 12/11/10 22:29, Jilles Tjoelker wrote:
So some more ideas:
A per-command LINENO does not require adding the number to all of the
node types. Only node types that are commands that perform expansions
need it: NCMD, NREDIR, NBACKGND, NSUBSHELL, NFOR, NCASE.
This makes sense, and I've tried this with good results. I added NDEFUN
to the list, to get the source to function line number mapping correct.
I noticed when wrapping half-parsed commands in an NREDIR, I got the
line number wrong (not necessarily nonconforming, but different from
what I had intended). I changed it in this patch by saving the line
number in a variable before starting the parsing of the command.
NFOR and NCASE may be wrapped in an NREDIR node to hold the line number.
This simplifies the code a bit at the cost of some extra memory usage.
That's possible. I have not used that idea in this patch, but it should
be an easy change.
Your other suggestions would work, but I am not able to say whether they
form an improvement, so I did not use those for now.
Again, comments are welcome.
diff --git a/src/error.c b/src/error.c
index e304d3d..e60e430 100644
--- a/src/error.c
+++ b/src/error.c
@@ -62,6 +62,7 @@ struct jmploc *handler;
int exception;
int suppressint;
volatile sig_atomic_t intpending;
+int errlinno;
static void exverror(int, const char *, va_list)
@@ -116,13 +117,12 @@ exvwarning2(const char *msg, va_list ap)
const char *fmt;
errs = out2;
- name = arg0 ?: "sh";
- fmt = "%s: ";
- if (commandname) {
- name = commandname;
+ name = arg0 ? arg0 : "sh";
+ if (!commandname)
fmt = "%s: %d: ";
- }
- outfmt(errs, fmt, name, startlinno);
+ else
+ fmt = "%s: %d: %s: ";
+ outfmt(errs, fmt, name, errlinno, commandname);
doformat(errs, msg, ap);
#if FLUSHERR
outc('\n', errs);
diff --git a/src/error.h b/src/error.h
index 3162e15..08f96e9 100644
--- a/src/error.h
+++ b/src/error.h
@@ -122,6 +122,7 @@ void onint(void) __attribute__((__noreturn__));
#else
void onint(void);
#endif
+extern int errlinno;
void sh_error(const char *, ...) __attribute__((__noreturn__));
void exerror(int, const char *, ...) __attribute__((__noreturn__));
const char *errmsg(int, int);
diff --git a/src/eval.c b/src/eval.c
index b966749..d85f66f 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -73,7 +73,7 @@
int evalskip; /* set if we are skipping commands */
STATIC int skipcount; /* number of levels to skip */
MKINIT int loopnest; /* current loop nesting level */
-static int funcnest; /* depth of function calls */
+STATIC int funcline; /* starting line number of current function, or
0 if not in a function */
char *commandname;
@@ -218,6 +218,9 @@ evaltree(union node *n, int flags)
status = !exitstatus;
goto setstatus;
case NREDIR:
+ errlinno = lineno = n->nredir.linno;
+ if (funcline)
+ lineno -= funcline - 1;
expredir(n->nredir.redirect);
pushredir(n->nredir.redirect);
status = redirectsafe(n->nredir.redirect, REDIR_PUSH);
@@ -296,7 +299,7 @@ calleval:
}
goto success;
case NDEFUN:
- defun(n->narg.text, n->narg.next);
+ defun(n);
success:
status = 0;
setstatus:
@@ -370,6 +373,10 @@ evalfor(union node *n, int flags)
struct strlist *sp;
struct stackmark smark;
+ errlinno = lineno = n->nfor.linno;
+ if (funcline)
+ lineno -= funcline - 1;
+
setstackmark(&smark);
arglist.lastp = &arglist.list;
for (argp = n->nfor.args ; argp ; argp = argp->narg.next) {
@@ -411,6 +418,10 @@ evalcase(union node *n, int flags)
struct arglist arglist;
struct stackmark smark;
+ errlinno = lineno = n->ncase.linno;
+ if (funcline)
+ lineno -= funcline - 1;
+
setstackmark(&smark);
arglist.lastp = &arglist.list;
expandarg(n->ncase.expr, &arglist, EXP_TILDE);
@@ -442,6 +453,10 @@ evalsubshell(union node *n, int flags)
int backgnd = (n->type == NBACKGND);
int status;
+ errlinno = lineno = n->nredir.linno;
+ if (funcline)
+ lineno -= funcline - 1;
+
expredir(n->nredir.redirect);
if (!backgnd && flags & EV_EXIT && !have_traps())
goto nofork;
@@ -697,6 +712,10 @@ evalcommand(union node *cmd, int flags)
int status;
char **nargv;
+ errlinno = lineno = cmd->ncmd.linno;
+ if (funcline)
+ lineno -= funcline - 1;
+
/* First expand the arguments. */
TRACE(("evalcommand(0x%lx, %d) called\n", (long)cmd, flags));
setstackmark(&smark);
@@ -730,7 +749,7 @@ evalcommand(union node *cmd, int flags)
*n
Re: [PATCH] Improved LINENO support
On Fri, Nov 12, 2010 at 08:07:41PM +0059, Harald van Dijk wrote: > must not write to test-7 and test2-8. It may write to test-1 and > test2-1, or it may write to test-8 and test2-8. Perhaps even to test-7 > and test2-7. Again, though, LINENO must be the same in both expansions. > So for dash, the problem doesn't change: storing the line number of each > word, rather than each command, makes it very hard to get LINENO right. Hmm, I hadn't thought of that. I considered a LINENO that points to the affected word most useful and easiest to implement (also because FreeBSD sh's LINENO works this way). So some more ideas: A per-command LINENO does not require adding the number to all of the node types. Only node types that are commands that perform expansions need it: NCMD, NREDIR, NBACKGND, NSUBSHELL, NFOR, NCASE. NFOR and NCASE may be wrapped in an NREDIR node to hold the line number. This simplifies the code a bit at the cost of some extra memory usage. A further simplification might be removing the redirect field from NBACKGND and NSUBSHELL and putting any redirect in an additional NREDIR node inside the NBACKGND/NSUBSHELL (so that the file is opened in the child process and job control works correctly). However, NREDIR, NBACKGND and NSUBSHELL are compatible types so it does not seem to gain much. What about making the lineno an element of the redirection list like NTO? This minimizes changes to eval.c but perhaps it is too contrived. Second idea: use a per-word LINENO but somehow ensure it is the same for all words in a command. I think this is wrong, but perhaps I'm wrong. -- Jilles Tjoelker -- To unsubscribe from this list: send the line "unsubscribe dash" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Improved LINENO support
On 12/11/10 20:08, Harald van Dijk wrote: On 12/11/10 19:35, Eric Blake wrote: On 11/12/2010 11:17 AM, Harald van Dijk wrote: ( : : : : : )>test-$LINENO Umm - there was more than one word in that example (each : in the subshell is a word) That was because it was a poor example. I meant to use comments, ...which don't work, because a command is required between ( and ). Never mind that part. -- To unsubscribe from this list: send the line "unsubscribe dash" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Improved LINENO support
On 12/11/10 19:35, Eric Blake wrote: On 11/12/2010 11:17 AM, Harald van Dijk wrote: ( : : : : : )>test-$LINENO should write to file test-1, not test-7, even though the only word is on line 7. bash gets this wrong, pdksh gets this almost right. Umm - there was more than one word in that example (each : in the subshell is a word). But I agree that the subshell command itself started on line 1, but the word containing $LINENO is on line 7. That was because it was a poor example. I meant to use comments, but forgot to change this before posting. I haven't checked if they count as words in SUS, but they don't in the dash source code. There are _so many_ variations on what this example would do that it's hard to say that any one version is definitively correct. In general, the standard tends to favor the ksh93 interpretation of things; but on your example, it touches the file test-6 (not test-7 nor test-1). > All I can see in the standard is: "Set by the shell to a decimal number representing the current sequential line number (numbered starting with 1) within a script or function before it executes each command." with no hint as to whether LINENO auto-increments over the number of newlines encountered while parsing that command. You may be right. SUS doesn't really specify whether "current" refers to the first or the last line number of a command. test-1 and test-7 are equally defensible, so I was too quick to say that bash gets this wrong. I hope you agree, though, that test-6 is iffy, and for my other example: echo $LINENO \ $LINENO two identical numbers must be printed. SUS only requires and only allows LINENO to be set before a command is executed, so once it is set, it is set for both expansions. Given that, ( # # # # # )>test-$LINENO \ >>test2-$LINENO must not write to test-7 and test2-8. It may write to test-1 and test2-1, or it may write to test-8 and test2-8. Perhaps even to test-7 and test2-7. Again, though, LINENO must be the same in both expansions. So for dash, the problem doesn't change: storing the line number of each word, rather than each command, makes it very hard to get LINENO right. -- To unsubscribe from this list: send the line "unsubscribe dash" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Improved LINENO support
On 11/12/2010 11:17 AM, Harald van Dijk wrote: > ( > : > : > : > : > : > ) >test-$LINENO > > should write to file test-1, not test-7, even though the only word is on > line 7. bash gets this wrong, pdksh gets this almost right. Umm - there was more than one word in that example (each : in the subshell is a word). But I agree that the subshell command itself started on line 1, but the word containing $LINENO is on line 7. There are _so many_ variations on what this example would do that it's hard to say that any one version is definitively correct. In general, the standard tends to favor the ksh93 interpretation of things; but on your example, it touches the file test-6 (not test-7 nor test-1). All I can see in the standard is: "Set by the shell to a decimal number representing the current sequential line number (numbered starting with 1) within a script or function before it executes each command." with no hint as to whether LINENO auto-increments over the number of newlines encountered while parsing that command. -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [PATCH] Improved LINENO support
On 10/11/10 22:59, Jilles Tjoelker wrote: What I was thinking of was adding a lineno field to narg instead of to all command types. The lineno variable would then be set by expand.c. I think that leads to a smaller patch and it should still give a sensible value for almost all errors. Unfortunately, I do not believe this will work. In echo $LINENO \ $LINENO two identical numbers must be printed, because LINENO is supposed to be set when a command is executed. It is not supposed to change during the execution of the command. Taking the first word of a command also will not work: ( : : : : : ) >test-$LINENO should write to file test-1, not test-7, even though the only word is on line 7. bash gets this wrong, pdksh gets this almost right. If I am missing an obvious way to address these issues, or if I am missing some part of the standard that clarifies why these issues do not exist, please share, but otherwise I will take the rest of your suggestions and rework my first patch, storing the line number for each command. -- To unsubscribe from this list: send the line "unsubscribe dash" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Improved LINENO support
On 10/11/10 22:59, Jilles Tjoelker wrote: What I was thinking of was adding a lineno field to narg instead of to all command types. The lineno variable would then be set by expand.c. I think that leads to a smaller patch and it should still give a sensible value for almost all errors. Downside is a few more int-to-ascii conversions. A few divisions is not that bad relative to other things that are done but printf in modern libcs is bloated and slow and might slow down things measurably. I made an attempt at this, but managed to keep the printing in one place by making LINENO a special variable, which is stored in an int and set in expandarg. When $LINENO is evaluated, the code checks to see if the variable is LINENO, and if so, converts the value to a string. This is one simple pointer comparison in the common case in lookupvar: if (v == &vlineno && v->text == linenovar) and avoids sprintf in scripts that don't use LINENO. FreeBSD's code subtracts the function's starting line number in the parser rather than at run time. I wonder which is best. (FreeBSD neglects to save and restore the value, so nested function definitions may lead to incorrect line numbers, but that could be fixed.) I used this approach in my updated patch, by resetting plinno to 0 in simplecmd, and restoring it after calling command(). I think this patch will increase dash's code size at least a little, which is always a consideration here. The code size varies little: the current limited LINENO support gives an executable size of 67680 bytes (default configure settings, no libedit, gcc 4.5.1, CFLAGS=-Os, strip after compilation), my previous patch gives 67688 bytes, my latest patch gives 67704 bytes. I do not think 24 bytes should be a problem. I can also look at getting this into FreeBSD sh. That would be great. I will send the updated patch when I can properly test it. Basic testing shows it works, but more extensive testing fails. As it turns out, this is unrelated to my changes: current git, unmodified, has a problem: $ cat test.sh echo cat <<_ACEOF $0 $@ _ACEOF echo $ src/dash test.sh wtf test.sh wtf � $ I had not noticed it with my earlier patch, because I actually did my testing with modified 0.5.6.1 sources. Thanks for the comments. Cheers, Harald -- To unsubscribe from this list: send the line "unsubscribe dash" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Improved LINENO support
On Mon, Nov 08, 2010 at 09:32:56PM +0059, Harald van Dijk wrote: > This patch improves LINENO support by storing line numbers in the parse > tree, for commands as well as for function definitions. It makes LINENO > behaves properly when calling functions, and has the added benefit of > improved line numbers in error messages when the last-parsed command is > not the last-executed one. It removes the earlier LINENO support, and > instead sets LINENO from evaltree when a command is executed. I have > used it for a while, and have seen correct line numbers in basic testing > and in error messages in fairly large and complicated autoconf-generated > shell scripts. Does this look good enough to add to dash? If there are > any problems with it, please let me know, I will be happy to look at > this further. What I was thinking of was adding a lineno field to narg instead of to all command types. The lineno variable would then be set by expand.c. I think that leads to a smaller patch and it should still give a sensible value for almost all errors. Downside is a few more int-to-ascii conversions. A few divisions is not that bad relative to other things that are done but printf in modern libcs is bloated and slow and might slow down things measurably. FreeBSD's code subtracts the function's starting line number in the parser rather than at run time. I wonder which is best. (FreeBSD neglects to save and restore the value, so nested function definitions may lead to incorrect line numbers, but that could be fixed.) I think this patch will increase dash's code size at least a little, which is always a consideration here. I can also look at getting this into FreeBSD sh. Thanks, -- Jilles Tjoelker -- To unsubscribe from this list: send the line "unsubscribe dash" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
