[Home]

Summary:ASTERISK-07403: labels for goto not working in macros
Reporter:Sherwood McGowan (rushowr)Labels:
Date Opened:2006-07-26 12:43:52Date Closed:2006-08-07 12:38:09
Priority:MinorRegression?No
Status:Closed/CompleteComponents:PBX/pbx_ael
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:
Description:If I make a label and call that label with a goto within a macro, I get an error. Here's a sample code block:
// Note the comments contained here are not in the original code
macro endcall(type) {
 switch(${type}) {
   case out:
     &nullchk(callid);
     if(${testnotnull}) {
       &endsess();
       goto ptr1 ; // <-- goto call to valid label
     }
     else {
ptr1: // <-- valid label
       Softhangup(${CHANNEL});
       break ;
     }
   Noop(esac) ;
 }
}

aelparse reports:
goto:  no label ptr1 exists in the current extension!

My current workaround (not always possible) is to do:

macro endcall(type) {
 switch(${type}) {
   case out:
     &nullchk(callid);
     if(${testnotnull}) {
       &endsess();
       &endcall(out);  // <-- Here's the change
     }
     else {
       Softhangup(${CHANNEL});
       break ;
     }
   Noop(esac) ;
 }
}

I'm not completely sure if this is a bug, but I've not been able to turn up evidence that labels are not useable in macros.
Comments:By: Serge Vecher (serge-v) 2006-07-26 12:58:32

murf, if you don't mind ... Thanks ;)
By: Steve Murphy (murf) 2006-07-26 18:19:37

Very good. I can reproduce this with the snippet supplied.
I can see right away that this problem is related to the fact that
the goto and target are mixed into a switch, which is implemented
with different extensions. I will puzzle this one out and fix it.
I just plain hadn't thought of this affect, but I should have...!

By: Steve Murphy (murf) 2006-07-26 19:28:00

Uh, you may not be very interested in details as such as what I am about to
regurgitate, but hey, this is part of the fun of filing a bug! The bug you reported was directly caused by a simple typo in the code that descends the
parse tree, which prevented it from descending far enough to find the label.

This was easily fixed, and the error message no longer appears...

But, as per my suspicions, the emitted code is incorrect, and will fail at run-time. Give me a little time, as the structure of the switch, vs how it is implemented in the underlying priorities, make this a really 'juicy' problem. It
can be solved, but to do it right is the trick. Right now, the "goto ptr1;" instruction turns into a Goto(s|ptr1) call. It should be Goto(sw-1-out|ptr1) to be correct in the generated code, because each case is output as a different priority. The code can do it, but it has to do it right; after all, the label could be in a switch in a switch in a switch, and in a different case of the switch in which the goto is located. So, the names have to be tracked carefully.

More to come.
By: Sherwood McGowan (rushowr) 2006-07-27 06:14:48

sounds good to me :)
Hope you don't mind, but I'll probably be posting bugs more often now. I've basically moved all my dialplan development over to AEL2. When I've got a client who doesn't want to patch in support for AEL2, I just develop locally and dump the dialplan on my dev server.

Also, is there any way I could contact you directly? sometimes I've got little questions about how AEL2 should behave, etc...
By: Sherwood McGowan (rushowr) 2006-07-27 06:16:06

oh and I definitely like to hear the gory details about why a bug exists, how it's being solved, etc... :) cheers
By: Steve Murphy (murf) 2006-07-27 12:16:57

Yes, let's talk, one way or another.

Call 1-307-754-5675, select 3, give the lady your name, and we can talk.
By: Steve Murphy (murf) 2006-07-28 09:35:07

OK, I've added another semantic check to AEL, to make sure you don't define the same label more than once within an extension. Hmmm. I did this because it needs to be done, otherwise you get problems either when you load it into asterisk, or at runtime; haven't investigated which, but I'd assume something would have to complain about that. I found a few more bugs in the checking traverser, which prevented some things from checking properly in macros.

In compiling AEL, there are two worlds, and two languages. The pval tree represents the AEL structure of the code. The ael_extension, ael_priority tree/list represents the extensions.conf view of the code. Compiling converts from one to other. AEL is bit more hierarchical, and the extensions.conf language a bit flatter.

Now, in a switch case/default/pattern, you have the complication that the contents are placed in a separate extension, to take advantage of the pattern matching facilitates inherent in extension names. A label in there will present challenges to properly forming a goto, and specifying the right extension. Now, the switch is "flattened" to a series of extensions, each extension title bearing unique idents mixed with the patterns the user specified in the case names. Predicting the unique ident would be possible, but messy. I think the right thing to do is to store the ref to the goto target(label) in both worlds, so a post processing phase can adjust the target extension name.

whew, I'm glad I got that off my chest... Hope ya'all are still awake!!
By: Sherwood McGowan (rushowr) 2006-07-28 09:56:13

Probably a dumb question, but are those changes available in the main trunk yet? If so, I'll pull it and test.

Also, a quick heads up, I'm going to submit a new bug concerning macro calls in AEL files that refer .conf language files. Figured since you were already in the macro portion of the parser... ;-) cheers Murf!
By: Steve Murphy (murf) 2006-08-01 00:22:27

OK, finally!! some big progress!! Here's what's happening:

1. I added fields to the various structs in ael_structs.h, to hold stuff like
  pointers to the label node as part of a goto structure;
2. boolean field on both the label and and the goto to indicate if the target
  is inside a case;
3. dad ptrs on all pval nodes, and code to set the father pointers in the
  parser. This was a big move up, as it makes storage of state information
  during the traversal largely uneccessary, and has all sorts of good
  affects for future development.
4. Restructured the way the asterisk extensions are generated. Instead of
  generating each extension as it's encountered during the parse tree
  traversal, and then destroying the extension, I needed to generate and keep
  all the extensions so I can process them in a subsequent pass and fix the
  goto instructions along the way. You see, the targets of goto's can be in
  other contexts and extensions, and we can neither reliably set the extension
  name if the name hasn't been generated yet, nor if it was generated and then
  unceremoniously deleted. Now I guess I'm doing a fairly true 2-pass
  compilation.
5. Generated extra functions to find the goto targets, check to see if a label
  is contained in a case somehow, get the containing context/extension/etc.
6. Put pointers in the pval structs to point to the compiled version of the
  label in the ael_priorities, and vice-versa for gotos.
7. Restructured several routines to facilitate doing what I wanted, when it
  needed doing, and the way it needed to be done.

Let me investigate it a little longer to make sure I didn't totally destroy things, and I'll be able to commit changes and let you test it.
By: Steve Murphy (murf) 2006-08-01 00:24:50

Oh, sorry, forgot to respond to your query above. No, I haven't submitted anything yet. No point in fixing one bug, only to have another surface and bother you deeply. It'd just be a waste of your time. But I'm a bit closer to having a general, working solution done... hang in there. Then we can get to the really fun stuff.
By: Steve Murphy (murf) 2006-08-01 10:53:06

Hello!!

I've run thru the regressions, found some small problems to added code, fixed them... dare I hope everything works?

the teams/murf/bug_7598 branch has all my changed committed. Try giving it a spin to see if macros are still buggy.
By: Steve Murphy (murf) 2006-08-01 12:39:03

I'll let this simmer until you can test it out... see if there's still any problems with macros and labels and gotos.


By: Sherwood McGowan (rushowr) 2006-08-01 14:01:22

Murf, I'll check it out as soon as possible. Thanks for jumpin' on this!
By: Steve Murphy (murf) 2006-08-02 09:53:23

OK, I just merged this into my home system's trunk checkout, and I can still make and get calls... So it doesn't appear I did anything grossly bad to AEL as yet. It's actually some quite obtuse AEL. If I did something bad, I'd expect to find out in a few milliseconds! How goes it for you, rushowr?
By: Sherwood McGowan (rushowr) 2006-08-02 22:55:44

Murf, I'll test this tomorrow on my vmware machine
By: Sherwood McGowan (rushowr) 2006-08-03 15:31:44

Murf, I pulled SVN-murf-bug_7598-r38839 and it compiled various macros containing goto/label statements. I dumped the dialplan via "save dialplan" and the 'compiled' dialplan code looks solid. Thanks for squashin' that cucaracha


By: Steve Murphy (murf) 2006-08-06 00:18:57

Hey, rushowr! How goes? Been able to test macros/labels out? They want to fork 1.4 this coming week; our opportunity to get the -w option into aelparse in 1.4 may pass, unless it's merged soon. Don't want to make you feel under any pressure, tho!! ;^)

murf


By: Sherwood McGowan (rushowr) 2006-08-06 01:09:39

Murf, my tests all came out great so I'd say  your code is solid. Is there something else I need to do for this bug? I'm going to go test your solution for 7606 now and report there :)
By: Steve Murphy (murf) 2006-08-07 12:37:00

OK, groovy, I've folded this into trunk. Many thanks for helping to test!
By: Steve Murphy (murf) 2006-08-07 12:38:09

And, I'm closing this bug. Hurray!!