[analyze] [CTU] Add support for pch paramteres of clang-extdef-mapping #3707

martong · 2022-07-13T16:30:06Z

This patch is about supporting https://reviews.llvm.org/D128704 from Clang.

When doing CTU analysis setup we parse the .cpp to get an .ast and then
we run clang-extdef-mapping on the .cpp file later. This is
sub-optimal since we have to re-parse the file again.

With this patch we can now run clang-extdef-mapping directly on
the .ast file. That saves some time.

Fixes #3703

martong · 2022-07-13T17:04:16Z

analyzer/tests/libtest/codechecker.py

+    """
+    try:
+        return bool(util.strtobool(
+            os.environ['CC_TEST_FORCE_EXTDEF_MAPPING_CAN_READ_PCH']))


@bruntib TODO set a JIRA ticket to turn this variable on before our next internal release, when we rebase Clang.

This should be done before the PR is merged.

martong · 2022-07-14T15:39:01Z

The overall CSA runtime (with the default profile) improved roughly with 1-2 % in case of C++, and with 0-1% in case of C.

martong · 2022-07-22T08:49:25Z

Ping

bruntib

Technically it looks good to me, though, it's not clear why relative/absolute paths matter. Maybe it would be worth for a comment about its reasons.

analyzer/codechecker_analyzer/analyzers/clangsa/ctu_autodetection.py

analyzer/codechecker_analyzer/analyzers/clangsa/config_handler.py

analyzer/codechecker_analyzer/analyzers/clangsa/ctu_autodetection.py

martong · 2022-07-27T12:39:44Z

Technically it looks good to me, though, it's not clear why relative/absolute paths matter. Maybe it would be worth for a comment about its reasons.

Thanks for the review! Relative/absolute paths matter because the "on-demand" and the "pch" based CTU uses the paths differently. This is documented in ctu_manager.py at the function func_map_list_src_to_ast:

    """ Turns textual function map list with source files into a
    mapping from mangled names to mapped paths, which can be absolute paths to
    the original source files if ctu_on_demand is True, or relative path
    segments to AST-dump files that reside in CTU-DIR directory otherwise. """

Fixes Ericsson#3703

martong · 2022-08-02T15:57:20Z

Ping

analyzer/codechecker_analyzer/analyzers/clangsa/ctu_manager.py

martong · 2022-09-26T14:27:07Z

Ping @Szelethus

Szelethus

This absolutely cannot go in without a proper PR description and commit message. Please make the review, and the subsequent archeology easier.

Does this have a corresponding LLVM commit and/or phabricator revision? Is there an issue or something for this? Why do we need this? Whats to gain?

martong · 2022-10-17T15:53:02Z

This absolutely cannot go in without a proper PR description and commit message. Please make the review, and the subsequent archeology easier.

Does this have a corresponding LLVM commit and/or phabricator revision? Is there an issue or something for this? Why do we need this? Whats to gain?

This patch is about supporting https://reviews.llvm.org/D128704 from Clang.

When doing CTU analysis setup we parse the .cpp to get an .ast and then
we run clang-extdef-mapping on the .cpp file later. This is
sub-optimal since we have to re-parse the file again.

With this patch we can now run clang-extdef-mapping directly on
the .ast file. That saves some time.

martong · 2022-10-17T15:55:29Z

BTW, the commit message refers to an issue that describes the meaning of this PR:
Fixes #3703

analyzer/codechecker_analyzer/analyzers/clangsa/ctu_autodetection.py

analyzer/codechecker_analyzer/analyzers/clangsa/ctu_manager.py

Szelethus · 2022-10-20T14:40:20Z

analyzer/codechecker_analyzer/analyzers/clangsa/ctu_manager.py

    # Make relative path out of absolute.
    path = path[1:] if path[0] == os.sep else path
+
+    # If clang-extdef-mapping can read a pch then the file entry already has
+    # the .ast suffix.


Is this something worth assertin then?

No, because there is another case we want to support: when the clang-extdef-mapping cannot read a pch (i.e. older Clang).

That is not what the comment says. It says "Can read pch => entry already has the .ast suffix". That is assertable, not that the entry has an .ast suffix.

I don't understand. In this line, we add a the ".ast" suffix for the path, but only when necessary.

analyzer/codechecker_analyzer/analyzers/clangsa/ctu_manager.py

Szelethus · 2022-10-20T14:50:00Z

analyzer/codechecker_analyzer/makefile.py

@@ -138,8 +138,12 @@ def __get_ctu_pre_analysis_cmds(self, action):
        cmds.append(' '.join(cmd))

        # Get command to create CTU index file.
+        # FIXME Even if clang-extdef-mapping supports pch files as parameters,
+        # feed the source files rather. This way, the below 'sed' command is
+        # still meaningful.


You lost me here a little. What is the context?

Unfortunately, there is a legacy technical debt here: The makefile generation implementation in makefile.py diverges and uses its own implementation for generating the ext-def-mapping file. E.g., Instead of using a generic version of ast-dump_path, it uses sed.

The FIXME states the current hack, or what the fix should be? I'm not in the clear. Shouldn't the FIXME state that we should use the ast_dump_path method? What is preventing us from doing it? Why does this divergence exist?

Also, what you wrote here could be in the code as well, right?

Oh, maybe this fixme should simply be in the documentation for get_extdef_mapping_cmd.

The FIXME states the current hack, or what the fix should be? I'm not in the clear.

makefile.py had been architectured with a technical debt.

Shouldn't the FIXME state that we should use the ast_dump_path method?

Ok, I am updating the comment with that.

What is preventing us from doing it? Why does this divergence exist?

That would require further changes in makefile.py which is out of the scope of this patch, in my opinion. Addressing the mentioned technical debt should be done in a follow up patch.

Also, what you wrote here could be in the code as well, right?

Could be, yes.

Oh, maybe this fixme should simply be in the documentation for get_extdef_mapping_cmd.

I think, this problem relates to makefile.py and not of the ctu_manager.

Szelethus · 2022-11-03T12:15:31Z

analyzer/codechecker_analyzer/analyzers/clangsa/ctu_manager.py

    """ AST-dump based analysis uses preprocessed paths, here the path prefix
    'ast' and the filename suffix '.ast' is added to the name of the original
    source file. """

+    # If clang-extdef-mapping can read a pch then the parameter 'path' is an
+    # absolute path to the ast file. Let's transform that to a relative path
+    # from the ctu-dir; that is the same format we have in the other cases.


I was kind of missing the "otherwise" part. "If [....] path is [...]. Otherwise, path is [...], which is what we want".

Ok, I tried to reword this.

Szelethus · 2022-11-03T12:46:17Z

analyzer/codechecker_analyzer/makefile.py

@@ -138,8 +138,12 @@ def __get_ctu_pre_analysis_cmds(self, action):
        cmds.append(' '.join(cmd))

        # Get command to create CTU index file.
+        # FIXME Even if clang-extdef-mapping supports pch files as parameters,
+        # feed the source files rather. This way, the below 'sed' command is
+        # still meaningful.


The FIXME states the current hack, or what the fix should be? I'm not in the clear. Shouldn't the FIXME state that we should use the ast_dump_path method? What is preventing us from doing it? Why does this divergence exist?

Also, what you wrote here could be in the code as well, right?

Szelethus · 2022-11-03T12:50:14Z

analyzer/codechecker_analyzer/analyzers/clangsa/ctu_manager.py

    # Make relative path out of absolute.
    path = path[1:] if path[0] == os.sep else path
+
+    # If clang-extdef-mapping can read a pch then the file entry already has
+    # the .ast suffix.


That is not what the comment says. It says "Can read pch => entry already has the .ast suffix". That is assertable, not that the entry has an .ast suffix.

Szelethus · 2022-11-03T12:56:29Z

analyzer/codechecker_analyzer/analyzers/clangsa/ctu_manager.py

-def get_extdef_mapping_cmd(action, config, source, func_map_cmd):
+def get_extdef_mapping_cmd(
+        action, config, source, func_map_cmd, triple_arch,
+        for_makefile_generation):


Please explain this parameter in the comments. I think I get it now -- we need to act a little different if this function is called from makefile.py -- explain why.

Ok, added. Plus I've added some more explanatory comments to the command assembly.

Szelethus · 2022-11-03T12:57:19Z

analyzer/codechecker_analyzer/makefile.py

@@ -138,8 +138,12 @@ def __get_ctu_pre_analysis_cmds(self, action):
        cmds.append(' '.join(cmd))

        # Get command to create CTU index file.
+        # FIXME Even if clang-extdef-mapping supports pch files as parameters,
+        # feed the source files rather. This way, the below 'sed' command is
+        # still meaningful.


Oh, maybe this fixme should simply be in the documentation for get_extdef_mapping_cmd.

Szelethus

LGTM! Very nice! 🥲

Szelethus · 2022-11-03T13:57:10Z

analyzer/codechecker_analyzer/makefile.py

+        # still meaningful. Unfortunately, there is a legacy technical debt
+        # here: The makefile generation implementation in makefile.py diverges
+        # and uses its own implementation for generating the ext-def-mapping
+        # file. E.g., Instead of using a generic version of ast-dump_path, it


Suggested change

# file. E.g., Instead of using a generic version of ast-dump_path, it

# file. E.g., Instead of using a generic version of ast_dump_path, it

Szelethus · 2022-11-03T14:01:41Z

analyzer/tests/libtest/codechecker.py

+    """
+    try:
+        return bool(util.strtobool(
+            os.environ['CC_TEST_FORCE_EXTDEF_MAPPING_CAN_READ_PCH']))


This should be done before the PR is merged.

martong requested a review from bruntib July 13, 2022 16:30

martong requested a review from vodorok as a code owner July 13, 2022 16:30

martong force-pushed the clang-extdef-mapping-pch branch from da76d5f to 9808e51 Compare July 13, 2022 16:44

martong mentioned this pull request Jul 13, 2022

Run clang-extdef-mapping on the .ast files instead of the source #3703

Open

martong commented Jul 13, 2022

View reviewed changes

bruntib added this to the release 6.20.0 milestone Jul 18, 2022

bruntib requested changes Jul 22, 2022

View reviewed changes

analyzer/codechecker_analyzer/analyzers/clangsa/ctu_autodetection.py Outdated Show resolved Hide resolved

analyzer/codechecker_analyzer/analyzers/clangsa/config_handler.py Show resolved Hide resolved

analyzer/codechecker_analyzer/analyzers/clangsa/ctu_autodetection.py Show resolved Hide resolved

Add support for pch paramteres of clang-extdef-mapping

a5ce565

Fixes Ericsson#3703

martong force-pushed the clang-extdef-mapping-pch branch from 9808e51 to a5ce565 Compare July 27, 2022 12:43

Szelethus self-requested a review August 17, 2022 07:39

whisperity changed the title ~~Add support for pch paramteres of clang-extdef-mapping~~ [analyze] [CTU] Add support for pch paramteres of clang-extdef-mapping Aug 19, 2022

whisperity added enhancement 🌟 analyzer 📈 Related to the analyze commands (analysis driver) clang sa 🐉 The Clang Static Analyzer is a source code analysis tool that finds bugs in C-family programs. labels Aug 19, 2022

whisperity reviewed Aug 19, 2022

View reviewed changes

analyzer/codechecker_analyzer/analyzers/clangsa/ctu_manager.py Outdated Show resolved Hide resolved

Szelethus modified the milestones: release 6.20.0, release 6.21.0 Sep 13, 2022

Szelethus requested changes Sep 29, 2022

View reviewed changes

Szelethus reviewed Oct 20, 2022

View reviewed changes

Gabor Marton added 3 commits November 2, 2022 18:06

Address Szelethus comments

f1194f6

Merge branch 'master' into clang-extdef-mapping-pch

58ec32f

Fix too many blank lines

d6c1c70

Szelethus reviewed Nov 3, 2022

View reviewed changes

Add more comments

229ccbd

Szelethus approved these changes Nov 3, 2022

View reviewed changes

Szelethus modified the milestones: release 6.21.0, release 6.22.0 Nov 7, 2022

dkrupp removed this from the release 6.22.0 milestone Apr 13, 2023

temyurchenko mentioned this pull request Jul 31, 2024

CTU mapping is not interpreted correctly llvm/llvm-project#101274

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[analyze] [CTU] Add support for pch paramteres of clang-extdef-mapping #3707

[analyze] [CTU] Add support for pch paramteres of clang-extdef-mapping #3707

martong commented Jul 13, 2022 •

edited

Loading

martong Jul 13, 2022

Szelethus Nov 3, 2022

martong commented Jul 14, 2022

martong commented Jul 22, 2022

bruntib left a comment

martong commented Jul 27, 2022

martong commented Aug 2, 2022

martong commented Sep 26, 2022

Szelethus left a comment •

edited

Loading

martong commented Oct 17, 2022

martong commented Oct 17, 2022

Szelethus Oct 20, 2022

martong Nov 2, 2022

Szelethus Nov 3, 2022

martong Nov 3, 2022

Szelethus Oct 20, 2022

martong Nov 2, 2022

Szelethus Nov 3, 2022

Szelethus Nov 3, 2022

martong Nov 3, 2022 •

edited

Loading

Szelethus Nov 3, 2022

martong Nov 3, 2022

Szelethus Nov 3, 2022

Szelethus Nov 3, 2022

Szelethus Nov 3, 2022

martong Nov 3, 2022

Szelethus Nov 3, 2022

Szelethus left a comment

Szelethus Nov 3, 2022

Szelethus Nov 3, 2022

	# file. E.g., Instead of using a generic version of ast-dump_path, it
	# file. E.g., Instead of using a generic version of ast_dump_path, it

[analyze] [CTU] Add support for pch paramteres of clang-extdef-mapping #3707

Are you sure you want to change the base?

[analyze] [CTU] Add support for pch paramteres of clang-extdef-mapping #3707

Conversation

martong commented Jul 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martong commented Jul 14, 2022

martong commented Jul 22, 2022

bruntib left a comment

Choose a reason for hiding this comment

martong commented Jul 27, 2022

martong commented Aug 2, 2022

martong commented Sep 26, 2022

Szelethus left a comment • edited Loading

Choose a reason for hiding this comment

martong commented Oct 17, 2022

martong commented Oct 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martong Nov 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Szelethus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martong commented Jul 13, 2022 •

edited

Loading

Szelethus left a comment •

edited

Loading

martong Nov 3, 2022 •

edited

Loading