Inbalanced `yy_preproc -> current_file` Stack

[zhanghongce]

Hi, again!

I think there might be an issue with the yy_preproc -> current_file stack when handling preprocessor macros (but not the include). For MACRO_IDENTIFIER, the lexer will jump into the macro definitions, but unlike the way that "include" is handled, it does not push the stack. At the end of that string, it will trigger the <<EOF>> case, which pops the stack. This could cause stack underflow, and when later yy_preproc -> current_file is used to assigned to the meta field, the file name there becomes null (the stack returns null when is underflowed.)

{MACRO_IDENTIFIER}     {

    // Look for the macro entry.
    verilog_macro_directive * macro = NULL;
    char * macroName = (yytext)+1;
    ast_hashtable_result r = ast_hashtable_get(yy_preproc -> macrodefines,
                                               macroName,
                                               (void**)&macro);
    
    if(r == HASH_SUCCESS)
    {
        // Switch buffers to expand the macro.

        YY_BUFFER_STATE cur = YY_CURRENT_BUFFER;
        YY_BUFFER_STATE n   = yy_scan_string(macro -> macro_value);
        
        yy_switch_to_buffer(cur);
        yypush_buffer_state(n);
    }
    else
    {
        // Undefined macro - PANIC!
        //printf("ERROR: Undefined macro '%s' on line %d\n", yytext, yylineno);
        //printf("\tIt's probably all going to fall apart now...\n\n");
    }
}

<<EOF>> {

    yypop_buffer_state();

    // We are exiting a file, so pop from the the preprocessor stack of files
    // being parsed.
    ast_stack_pop(yy_preproc -> current_file);


    if ( !YY_CURRENT_BUFFER )
    {
        yyterminate();
    }
    else
    {
        YY_BUFFER_STATE cur = YY_CURRENT_BUFFER;
        yylineno = cur -> yy_bs_lineno;
    }
}

[ben-marshall]

Hello! Yes, this looks like a bug for sure, well found.

Intuition tells me correct behavior would be to add a push to the stack when entering a MACRO_IDENTIFIER. That way, if a problem occurs inside the syntax parsed as part the macro, you're more likely to get a proper error message about where the error comes from. The pop code can then stay as normal.

Does that sound sensible?

I'll try a test case and fix when I get the chance.

[zhanghongce]

Thanks for your reply!

Yes, I think that should work!

[ben-marshall]

So, I've opened up #24 .

This PR fixes the original problem in #23, but it makes it very obvious how poorly I implemented all of the memory management. Long term, I'd like to actually remove most of the AST construction code from YACC source code, and replace them with function pointers to hooks people can override / implement themselves.
Memory management / data structure management then becomes a problem for the library user. It'll be extra effort for them, but they'd only need to grab from the parser info which they needed, rather than the entire tree.

@zhanghongce - I don't know what you're view on something like this would be, given you've given me more feedback on this code than everyone else combined?

If there are no objections, I'll merge #24 and close this issue, pending a big re-write of the repo to fix the memory management stuff as above.

[zhanghongce]

@ben-marshall Thanks for the fix!

As for the big plan, sorry that I don't have a clear idea on what should be done. When I use it in my project, there was a time that I think it might be a great help to me if the data structures are modern (C++ fashion), so I don't need to panic when I get a void * pointer from ast_list and don't know what type exactly I should try to cast it to and whether that cast is a good one or not. (I mean at least I can try to do a dynamic cast and know when I convert it to a wrong type).

So in my opinion, allowing users to replace the hooks and implement their own data structure could solve this, though it comes with extra work for the users.

But, all in all, I greatly appreciate the work you have done on setting up the data structures. That is very helpful to understand how Verilog should be handled!

Thank you!