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

add --gcc-analyzer-bin option to gcc plug-in #41

Closed
wants to merge 8 commits into from
Closed

add --gcc-analyzer-bin option to gcc plug-in #41

wants to merge 8 commits into from

Conversation

sibeream
Copy link
Contributor

@sibeream sibeream commented Oct 4, 2021

No description provided.

Copy link
Member

@kdudka kdudka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think this could really work without updating props.env["CSGCCA_ANALYZER_BIN"]. Minor review comments inline...

py/plugins/gcc.py Outdated Show resolved Hide resolved
py/plugins/gcc.py Outdated Show resolved Hide resolved
py/plugins/gcc.py Outdated Show resolved Hide resolved
@sibeream
Copy link
Contributor Author

I do not think this could really work without updating props.env["CSGCCA_ANALYZER_BIN"]. Minor review comments inline...

I deleted it by accident. Should work as intended now. I'm sorry for the confusion.

@sibeream sibeream requested a review from kdudka October 11, 2021 10:43
Copy link
Member

@kdudka kdudka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update! Unfortunately, it does not work as expected. If a direct absolute path to the DTS gcc binary is given to --gcc-analyzer-bin, gcc is not invoked through cswarp, which is otherwise responsible for capturing the diagnostic output. A few review comments inline...

py/plugins/gcc.py Outdated Show resolved Hide resolved
py/common/util.py Outdated Show resolved Hide resolved
py/plugins/gcc.py Outdated Show resolved Hide resolved
py/plugins/gcc.py Outdated Show resolved Hide resolved
@kdudka
Copy link
Member

kdudka commented Oct 12, 2021

This helped me to make it actually work:

  1. Create an executable script /usr/bin/csmock-gcc-wrapper in the buildroot with the following content:
#!/bin/bash
exec /opt/rh/gcc-toolset-11/root/usr/bin/gcc "$@"
  1. Create a symlink /usr/lib64/cswrap/csmock-gcc-wrapper -> ../../bin/cswrap in the buildroot
  2. Give --gcc-analyzer-bin=csmock-gcc-wrapper to csmock.

If this is the way to go, we need to figure out how to provide the functionality through a user-friendly and flexible enough interface...

@sibeream
Copy link
Contributor Author

This helped me to make it actually work:

1. Create an executable script `/usr/bin/gcc-11` in the buildroot with the following content:
#!/bin/bash
exec /opt/rh/gcc-toolset-11/root/usr/bin/gcc "$@"
1. Create a symlink `/usr/lib64/cswrap/gcc-11` -> `../../bin/cswrap` in the buildroot

2. Give `--gcc-analyzer-bin=gcc-11` to csmock.

If this is the way to go, we need to figure out how to provide the functionality through a user-friendly and flexible enough interface...

The solution which came to me is to inject /opt/rh/gcc-toolset-11/root/usr/bin/ in PATH variable right after cswrap and use gcc as analyzer name in this code:
https://github.com/csutils/cscppc/blob/d2c4a2ec48675d8777e4631e08092f80ab3ef58e/src/cswrap-core.c#L396

I.e. --gcc-analyzer-bin and CSGCCA_ANALYZER_BIN should provide not the path to analyzer binary, but the path to the directory, containing binary named gcc, which will be used as analyzer.

The limitation created by this solution would be that binary has to be named gcc, however since analyzer flags are hardcoded in csgcca in the way they are used in GCC, I would say we already limit options for binary to be GCC or at least to interpret flags the same way GCC doing. Which in my opinion effectively means that we expect analyzer to be GCC.

Also per my understanding in execution branch where analyzer is actually executed, we don't expect the system compiler to be executed in the future, so every process that reached the point to execute analyzer can safely mask all traces of the system compiler in the PATH variable.

@kdudka
Copy link
Member

kdudka commented Oct 13, 2021

csgcca by definition expects the tool to be GCC analyzer. On the other hand, it does not mean that the binary has to be named gcc. The binary can be named gcc-10.3.0 or x86_64-redhat-linux-g++. If we make --gcc-analyzer-bin take absolute path (and document it such), the gcc plug-in can take care of everything without unnecessary limitations. Am I missing anything?

-fixes after review
@sibeream sibeream requested a review from kdudka October 18, 2021 15:59
Copy link
Member

@kdudka kdudka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update!

CSMOCK_GCC_WRAPPER_NAME needs to be appended to ${CSWRAP_TIMEOUT_FOR} in order to keep the --cswrap-timeout option of csmock working. Two minor remarks inline.

py/plugins/gcc.py Outdated Show resolved Hide resolved
py/plugins/gcc.py Outdated Show resolved Hide resolved
@sibeream sibeream requested a review from kdudka October 25, 2021 14:42
@kdudka
Copy link
Member

kdudka commented Oct 25, 2021

Thanks for the update! It seems to work as expected now.

I have added just three comments to explain how --gcc-analyzer-bin is implemented:

--- a/py/plugins/gcc.py
+++ b/py/plugins/gcc.py
@@ -185,27 +185,30 @@ class Plugin:
                     results.error("`%s -fanalyzer` does not seem to work, "
                                   "disabling the tool" % analyzer_bin, ec=0)
                     return 0

                 if args.gcc_analyzer_bin:
+                    # create an executable shell script to wrap the custom gcc binary
                     wrapper_script = CSMOCK_GCC_WRAPPER_TEMPLATE % analyzer_bin
                     cmd = "echo '%s' > %s && chmod 755 %s" % \
                           (wrapper_script,
                            CSMOCK_GCC_WRAPPER_PATH,
                            CSMOCK_GCC_WRAPPER_PATH)
                     rv = mock.exec_chroot_cmd(cmd)
                     if 0 != rv:
                         results.error("failed to create csmock gcc wrapper script")
                         return rv

+                    # wrap the shell script by cswrap to capture output of `gcc -fanalyzer`
                     cmd = "ln -sf ../../bin/cswrap %s/%s" % \
                           (props.cswrap_path, CSMOCK_GCC_WRAPPER_NAME)
                     rv = mock.exec_chroot_cmd(cmd)
                     if 0 != rv:
                         results.error("failed to create csmock gcc wrapper symlink")
                         return rv

+                    # tell csgcca to use the wrapped script rather than system gcc analyzer
                     props.env["CSGCCA_ANALYZER_BIN"] = CSMOCK_GCC_WRAPPER_NAME

                 # XXX: changing props this way is extremely fragile
                 # insert csgcca right before cswrap to avoid chaining
                 # csclng/cscppc while invoking `gcc -fanalyzer`

Let's merge this as it is and continue with the development via separate pull requests.

@kdudka kdudka closed this in da68dd4 Oct 25, 2021
@kdudka kdudka changed the title add --gcc-analyzer-package and --gcc-analyzer-bin flags to gcc plugin add --gcc-analyzer-bin option to gcc plug-in Oct 25, 2021
TomasBeranek pushed a commit to TomasBeranek/csmock that referenced this pull request Oct 27, 2021
... to specify a custom build of gcc to be used for `gcc -fanalyzer`

Closes: csutils#41
@sibeream sibeream deleted the gcc-dts branch November 8, 2021 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants