NASM - Official Recursive Macro Support!

[Keith Kanios]

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.

Pushed a simple fix for this.

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?

No, but the latest push does.

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.

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.

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.

What is the advantage/benefit of this?

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.

Define properly used.

[cm]

No, but the latest push does.

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?

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.

Okay. I'll report back, if I should get something that works (and handles all cases well).

What is the advantage/benefit of this?

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.

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.

Define properly used.

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]

No, but the latest push does.

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?

Not sure. If you can work up an example that breaks it, I'll take a look at it.

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.

Okay. I'll report back, if I should get something that works (and handles all cases well).

OK.

What is the advantage/benefit of this?

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.

Can probably simplify that as mark, then free in pp_getline:

Code: [Select]

} else if ((ed->type != EXP_MMACRO) || (ed->unlink == true)) {

Now, do we want to prevent use of a macro that is marked in such a manner?

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.

Define properly used.

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).)

Can't remember what expansions was implemented for, probably safe to remove.

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.)

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.

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?

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]

Can probably simplify that as mark, then free in pp_getline:

Code: [Select]

} else if ((ed->type != EXP_MMACRO) || (ed->unlink == true)) {


No, we have to check whether this is the last invocation of the macro that terminated. Here's my current code.

Code: [Select]

/*
 * 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
};

This is in pp_getline:

Code: [Select]

                        /*
                         * 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);
                        }

This is in do_directive (%unmacro):

Code: [Select]

                *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;
                }


Now, do we want to prevent use of a macro that is marked in such a manner?

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.

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.

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.