Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Call graph regression: Some parts are mixed up #8476

Closed
benjarobin opened this issue Apr 3, 2021 · 16 comments
Closed

Call graph regression: Some parts are mixed up #8476

benjarobin opened this issue Apr 3, 2021 · 16 comments

Comments

@benjarobin
Copy link

benjarobin commented Apr 3, 2021

Describe the bug
For a C project, when doxygen is generating the call graph with dot (CALL_GRAPH and CALLER_GRAPH are set to YES), the resulting html documentation contains call graph and caller graph from the wrong function, even if the root node is the correct function name. Everything looks mixed up in the call tree...

After running git bisect start origin/master tags/Release_1_8_20 I found that the regression was introduces by the commit 0252f61, the commit e9ca9dc is fine.

So the version 1.9.1 is impacted.

Expected behavior
Correct call graph / caller graph generation.

To Reproduce
test-repro.zip

Screenshot OK
(using commit e9ca9dc)
ok

Screenshot KO
(using commit 0252f61)
ko

Version
See bisect analyze above. Can be reproduced with latest master

@albert-github
Copy link
Collaborator

From your bisect I assume that the problem is still present in the current master.

  • Please try to generate an example so it possible for us to reproduce the problem.

@benjarobin
Copy link
Author

@albert-github I was able to generate a very small example (kind of dirty...). And updated the first message with it.

@albert-github
Copy link
Collaborator

Thanks for the example, this would otherwise be hard / impossible to check.

@albert-github
Copy link
Collaborator

I've able to reduce the problem a little bit further to:

void ExecState7(void)
{

}

void MGR_Process(void)
{
   ExecState7();
}

struct MGR_CmdOrder_t MGR_GetCmdOrder(void)
{
    return m_cmdOrder;
}

and a Doxyfile:

EXTRACT_ALL            = YES
QUIET                  = YES
GENERATE_LATEX         = NO
HAVE_DOT               = YES
CALL_GRAPH             = YES
CALLER_GRAPH           = YES

Looks like it depends on the return type

  • when I change the return type struct MGR_CmdOrder_t to int the result is OK
  • when I change the return type of MGR_Process to struct MGR_CmdOrder_t there is no call / caller graph at all

Example: example.tar.gz

@benjarobin
Copy link
Author

benjarobin commented Apr 3, 2021

The code was previously "buggy", but the bug was hidden. Even in commit e9ca9dc, the addSourceRef() function is called with the wrong line number. In my example 2 functions are put in the same location (line number).

Basically every function/variable returning a "complex type" (an enum or a struct), is not located properly (not at the right line number).

Edit 1: The function DefinitionImpl::setBodySegment(int defLine, int bls,int ble) is called with defLine wrong value, but bls and ble are rights value

@albert-github
Copy link
Collaborator

Well noted. I had seen it as well but didn't pay attention to it.

Some further tests show that doxygen doesn't like a struct (or class or ...?) as a return type. In scanner.l it looks like doxyen thinks that a new struct starts instead of a return type. Apparently the lower level handling changed due to the mentioned commit (as scanner.l didn't change).

@benjarobin
Copy link
Author

This is definitively a problem inside scanner.l, but I do not know the proper fix for it...
Nonetheless, applying this fix works for my use case. In <CompoundName>{SCOPENAME} section:

                                           yyextra->current->name = yytext ;
                                           if (yyextra->clangParser && (yyextra->insideCpp || yyextra->insideObjC))
                                           {
                                             yyextra->current->id = yyextra->clangParser->lookup(yyextra->yyLineNr,yytext);
                                           }
+                                          yyextra->yyBegColNr=yyextra->yyColNr;
+                                          yyextra->yyBegLineNr=yyextra->yyLineNr;
                                           lineCount(yyscanner);
                                           if (yyextra->current->spec & Entry::Protocol)
                                           {
                                             yyextra->current->name += "-p";
                                           }

@benjarobin
Copy link
Author

@albert-github @doxygen Any idea how to fix properly this problem?

@albert-github
Copy link
Collaborator

This looks like to fix the problem for this case, though I'm a bit hesitant for side effects as not the complete type is processed

The mentioned code should be added around, currently, line 5604 where the compound name MGR_GetCmdOrder is handled. The word struct has been been handled at line 1489, rule:

<FindMembers>{B}*{TYPEDEFPREFIX}"struct{" |
<FindMembers>{B}*{TYPEDEFPREFIX}"struct"/{BN}+

in the rule at line 5598 the second part of the type (MGR_CmdOrder_t) is handled and the function name (MGR_CmdOrder) is handled at 4624 with the rule:

<ClassVar>{SCOPENAME}{BNopt}/"("

it looks like th efunction addType sees to it that the type is OK though.

When we create function:

int MGR_GetCmdOrder_int(void)
{
    return 42;
}

and this type is handled around line 2173 with the rule:

<FindMembers,FindMemberName>{SCOPENAME}

here the int and MGR_GetCmdOrder_int are handled.

@benjarobin
Copy link
Author

Any new ? Without any fixes a recent Doxygen version is not usable.

@doxygen
Copy link
Owner

doxygen commented Aug 17, 2021

@benjarobin I've just pushed a potential fix. Please verify if it works for you.

@albert-github
Copy link
Collaborator

I did a quick test with the original test case (see #8476 (comment)), but it looks like I'm missing the caller graphs for the different ExecState functions (compare the output of the current master version, 1.9.2 (0e3174e), with the 1.8.20 release).

@doxygen
Copy link
Owner

doxygen commented Aug 17, 2021

@albert-github Indeed, the same logic bug that was there for callgraphs was also there for calledby graphs. Should be fixed now.

@albert-github
Copy link
Collaborator

Looks OK now.

I also looked at the:

REFERENCED_BY_RELATION = YES
REFERENCES_RELATION    = YES

looks OK too though a bit strange, for me, is the behavior for the enum values:

image

image

so in one place there is a "References" but on the other place there is no "Referenced by ". I'm not sure what would be best, also in relation to e.g. the m_stateMachine.

@albert-github albert-github added the fixed but not released Bug is fixed in github, but still needs to make its way to an official release label Aug 17, 2021
@benjarobin
Copy link
Author

benjarobin commented Aug 17, 2021

@doxygen Thank you! I built doxygen from master, then test it on my "big" project, and everything looks good :-)
Also thanks to @albert-github

@doxygen
Copy link
Owner

doxygen commented Aug 18, 2021

This issue was previously marked 'fixed but not released',
which means it should be fixed in doxygen version 1.9.2.
Please verify if this is indeed the case. Reopen the
issue if you think it is not fixed and please include any additional information
that you think can be relevant (preferably in the form of a self-contained example).

@doxygen doxygen removed the fixed but not released Bug is fixed in github, but still needs to make its way to an official release label Aug 18, 2021
@doxygen doxygen closed this as completed Aug 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants