NASM Forum > Announcements
NASM - Official Recursive Macro Support!
Keith Kanios:
--- Quote from: cm on December 18, 2010, 07:16:48 PM ---
--- Quote from: Keith Kanios on December 18, 2010, 04:58:40 PM ---
--- Quote from: cm on November 11, 2010, 12:57:31 PM ---Notably, several warnings about unterminated strings are displayed after periods of waiting. Investigating these, I found that they were generated by "code" in %if 0 blocks, which I use for long comments because of the lacking %comment in previous releases.
--- End quote ---
Pushed a simple fix for this.
--- End quote ---
I've looked into the source myself and did not yet implement a good solution. In your fix, are these error conditions still handled properly when expanding non-%comment ExpDefs later?
--- End quote ---
No, but the latest push does.
--- Quote from: cm on December 18, 2010, 07:16:48 PM ---Besides, it isn't optimal that tokenize() is always used - even if the (untokenized, ie input) line was only read to be discarded. Anything except certain preproc directives is discarded in %comment blocks, and ignored %if parts (and some other less common cases). My current method of checking for whitespace, then a preproc ID with a letter, and discarding the line if it doesn't look like that, actually decreases performance (by a noticeable amount!) in some reference cases though. A better method might be to check there only for the specific directives and processing them directly if an expansion is currently defined. But implementing that well might require understanding the hash table creation and lookup, which I don't. Just rambling here.
--- End quote ---
Yes, I have come to the same conclusion. I am currently in "make it work" mode and will revisit this for "make it better" mode.
--- Quote from: cm on December 18, 2010, 07:16:48 PM ---Your fix for %unmacro on a currently active macro is okay but I'm currently playing around with a mechanism where the check that frees any ExpDef except macro ones will additionally check for a certain flag - set by %unmacro if it would display your error. I store that flag in the `state' field as that's only used by %if (and maybe %while).
I'll probably get that %unmacro patch (along with a bunch of other preproc patches) ready during the holidays.
--- End quote ---
What is the advantage/benefit of this?
--- Quote from: cm on December 18, 2010, 07:16:48 PM ---EDIT: By the way, I think the linked list `expansions' wasn't properly used anywhere in the repo. Just confused me a little when I reviewed/compared one of your patches right now.
--- End quote ---
Define properly used.
cm:
--- Quote from: Keith Kanios on December 18, 2010, 08:15:09 PM ---No, but the latest push does.
--- End quote ---
What if the processed line is a preproc directive that ends/changes/goes deeper, though? In most simple cases (like %endcomment) these will just check that the line ends after the directive, but in other cases (think %elif) such a line might actually be parsed. What happens then if there is an unterminated %[ construct?
--- Quote from: Keith Kanios on December 18, 2010, 08:15:09 PM ---Yes, I have come to the same conclusion. I am currently in "make it work" mode and will revisit this for "make it better" mode.
--- End quote ---
Okay. I'll report back, if I should get something that works (and handles all cases well).
--- Quote from: Keith Kanios on December 18, 2010, 08:15:09 PM ---What is the advantage/benefit of this?
--- End quote ---
I don't think it has an intrinsic one - an error might help users to catch obscure errors; support for it (possibly with a (suppressible) warning - I don't show one) eases some things. But note that you partially break compatibility (not that this is such a bad thing in a major new release); previously, %unmacro on an active macro appeared to work fine - the only issue, as I reported, appeared to be that the macro's name would not show correctly in warnings/errors caused by that macro's content. In my implementation of this, %unmacro will immediately unlink the ExpDef from the list of macros but will not free it just yet if it is still active. The code that frees ExpDefs will free a macro's ExpDef only if it reached cur_depth == 0 and the state was set by %unmacro as I mentioned; all other ExpDefs are freed unconditionally.
--- Quote from: Keith Kanios on December 18, 2010, 08:15:09 PM ---
--- Quote from: cm on December 18, 2010, 07:16:48 PM ---EDIT: By the way, I think the linked list `expansions' wasn't properly used anywhere in the repo. Just confused me a little when I reviewed/compared one of your patches right now.
--- End quote ---
Define properly used.
--- End quote ---
I didn't review the code in the repo again (yet, but I don't really intend to) but I think it wasn't read anywhere really. `istk->expansion' is the linked list of currently active expansions while `defining' points to the currently defined expansion. `expansions' is unnecessary and not used. In my current source I removed it entirely. (Actually I see that the variable still existed, but it wasn't accessed anywhere (in my modified source).)
In your documentation push you state that %while<condition> is implemented. I think the appropriate directives are checked for already, but the code (for anything but plain %while) is not yet implemented. If you intend to change that, you'll have to think about how to store the %while's condition type in the ExpDef. (You could also go with the current implementation's way to store the actual condition and re-parse the necessary part of the stored condition line each time the condition is to be evaluated.)
This brings me to another thing I thought about: There are a number of fields in ExpDefs and ExpInvs that are only used for specific expansion types (often, these fields are specific to macros). Do you think that it would be worth it to do something about that?
Keith Kanios:
--- Quote from: cm on December 18, 2010, 09:33:34 PM ---
--- Quote from: Keith Kanios on December 18, 2010, 08:15:09 PM ---No, but the latest push does.
--- End quote ---
What if the processed line is a preproc directive that ends/changes/goes deeper, though? In most simple cases (like %endcomment) these will just check that the line ends after the directive, but in other cases (think %elif) such a line might actually be parsed. What happens then if there is an unterminated %[ construct?
--- End quote ---
Not sure. If you can work up an example that breaks it, I'll take a look at it.
--- Quote from: cm on December 18, 2010, 09:33:34 PM ---
--- Quote from: Keith Kanios on December 18, 2010, 08:15:09 PM ---Yes, I have come to the same conclusion. I am currently in "make it work" mode and will revisit this for "make it better" mode.
--- End quote ---
Okay. I'll report back, if I should get something that works (and handles all cases well).
--- End quote ---
OK.
--- Quote from: cm on December 18, 2010, 09:33:34 PM ---
--- Quote from: Keith Kanios on December 18, 2010, 08:15:09 PM ---What is the advantage/benefit of this?
--- End quote ---
I don't think it has an intrinsic one - an error might help users to catch obscure errors; support for it (possibly with a (suppressible) warning - I don't show one) eases some things. But note that you partially break compatibility (not that this is such a bad thing in a major new release); previously, %unmacro on an active macro appeared to work fine - the only issue, as I reported, appeared to be that the macro's name would not show correctly in warnings/errors caused by that macro's content. In my implementation of this, %unmacro will immediately unlink the ExpDef from the list of macros but will not free it just yet if it is still active. The code that frees ExpDefs will free a macro's ExpDef only if it reached cur_depth == 0 and the state was set by %unmacro as I mentioned; all other ExpDefs are freed unconditionally.
--- End quote ---
Can probably simplify that as mark, then free in pp_getline:
--- Code: ---} else if ((ed->type != EXP_MMACRO) || (ed->unlink == true)) {
--- End code ---
Now, do we want to prevent use of a macro that is marked in such a manner?
--- Quote from: cm on December 18, 2010, 09:33:34 PM ---
--- Quote from: Keith Kanios on December 18, 2010, 08:15:09 PM ---
--- Quote from: cm on December 18, 2010, 07:16:48 PM ---EDIT: By the way, I think the linked list `expansions' wasn't properly used anywhere in the repo. Just confused me a little when I reviewed/compared one of your patches right now.
--- End quote ---
Define properly used.
--- End quote ---
--- Quote from: cm on December 18, 2010, 09:33:34 PM ---I didn't review the code in the repo again (yet, but I don't really intend to) but I think it wasn't read anywhere really. `istk->expansion' is the linked list of currently active expansions while `defining' points to the currently defined expansion. `expansions' is unnecessary and not used. In my current source I removed it entirely. (Actually I see that the variable still existed, but it wasn't accessed anywhere (in my modified source).)
--- End quote ---
--- End quote ---
Can't remember what expansions was implemented for, probably safe to remove.
--- Quote from: cm on December 18, 2010, 09:33:34 PM ---In your documentation push you state that %while<condition> is implemented. I think the appropriate directives are checked for already, but the code (for anything but plain %while) is not yet implemented. If you intend to change that, you'll have to think about how to store the %while's condition type in the ExpDef. (You could also go with the current implementation's way to store the actual condition and re-parse the necessary part of the stored condition line each time the condition is to be evaluated.)
--- End quote ---
I don't intend to expand on it. Feel free to rework it so that it is more like the %if family, if the rest of the dev team approves.
--- Quote from: cm on December 18, 2010, 09:33:34 PM ---This brings me to another thing I thought about: There are a number of fields in ExpDefs and ExpInvs that are only used for specific expansion types (often, these fields are specific to macros). Do you think that it would be worth it to do something about that?
--- End quote ---
Yes, once the current effort is "stable" and we know the processor is not going to change much further, we can revisit such optimizations.
cm:
--- Quote from: Keith Kanios on December 19, 2010, 06:06:56 PM ---Can probably simplify that as mark, then free in pp_getline:
--- Code: ---} else if ((ed->type != EXP_MMACRO) || (ed->unlink == true)) {
--- End code ---
--- End quote ---
No, we have to check whether this is the last invocation of the macro that terminated. Here's my current code.
--- Code: ---/*
* The state field of mmacro ExpDefs contains one of these
* values. "Free defered" indicates that this mmacro should
* be freed by its last invocation's termination (like other
* ExpDefs are anyway). This is set by %unmacro if necessary.
*/
enum {
MACRO_NORMAL = 0, MACRO_FREEDEFERED
};
--- End code ---
This is in pp_getline:
--- Code: --- /*
* The expansion that ran out is a %while or %rep that
* terminated, or an %if construct wrote all its content,
* or a multi-line macro invocation completed.
*/
if (ed->cur_depth > 0)
ed->cur_depth --;
else
nasm_assert(ei->type == EXP_IF);
/*
* Free the expansion's definition if so requested.
* This frees the ExpDefs of all %rep, %while and %if
* constructs. Additionally, %unmacro will set its
* macro's state to "Free defered" in case the macro
* is currently expanding. (%comment does not actually
* create invocations; %final and pre-defs do create
* such but are anonymous (ie they have no ExpDef).)
*
* Note that entered macros are only freed here if we
* are the last invocation to terminate. All of the
* other expansion types are only invoked once; the
* termination of that single invocation therefore
* unconditionally marks they should be freed.
*/
if (ed->type != EXP_MMACRO
|| (ed->state == MACRO_FREEDEFERED && ed->cur_depth == 0)) {
free_expdef(ed);
}
--- End code ---
This is in do_directive (%unmacro):
--- Code: --- *ed_p = ed->next;
/*
* Free the macro's definition if it isn't expanding, or defer
* freeing it until all of the macro's expansions terminate. Note
* that we unlinked the definition anyway, so new expansions won't
* use the definition anymore.
*/
if (!ed->cur_depth)
free_expdef(ed);
else {
ed->state = MACRO_FREEDEFERED;
}
--- End code ---
--- Quote from: Keith Kanios on December 19, 2010, 06:06:56 PM ---Now, do we want to prevent use of a macro that is marked in such a manner?
--- End quote ---
In the above code fragment and comment I note that usage of the macro (ie new invocations) are immediately made impossible in the %unmacro handler by unlinking it from the list. I think that this is the right behaviour, and it is efficiently and simply implemented by always unlinking the ExpDef.
--- Quote from: Keith Kanios on December 19, 2010, 06:06:56 PM ---I don't intend to expand on it. Feel free to rework it so that it is more like the %if family, if the rest of the dev team approves.
--- End quote ---
Will do so later on, but if a release/RC goes out with the current implementation then the doc could explicitly say that only %while is implemented.
Navigation
[0] Message Index
[*] Previous page
Go to full version