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.

Sign up for GitHub

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

Fix resource dir issue #1173

Merged
merged 5 commits into from
Nov 27, 2017
Merged

Fix resource dir issue #1173

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fc0ab38
Fix resource dir issue
Nov 23, 2017
906f399
Use clang bin to get clang-tidy resource_dir
Nov 24, 2017
ea1a067
Move resource_dir system include after gcc system includes
Nov 24, 2017
6ad66cd
Add codecs.decode in create_dependencies
Nov 27, 2017
fce793f
Overwrite PATH only if binary is abs
Nov 27, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions libcodechecker/analyze/analysis_manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
"""

from collections import defaultdict
import codecs
import glob
import multiprocessing
import os
Expand Down Expand Up @@ -148,6 +149,7 @@ def __eliminate_argument(arg_vect, opt_strings, num_args=0):
output, rc = util.call_command(command,
env=os.environ,
cwd=action.directory)
output = codecs.decode(output, 'utf-8', 'replace')
if rc == 0:
# Parse 'Makefile' syntax dependency output.
dependencies = output.replace('__dummy: ', '') \
Expand Down
17 changes: 13 additions & 4 deletions libcodechecker/analyze/analyzer_env.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,8 @@ def get_check_env(path_env_extra, ld_lib_path_extra):
return new_env


def extend_analyzer_cmd_with_resource_dir(analyzer_cmd, compiler_resource_dir):
def extend_analyzer_cmd_with_resource_dir(analyzer_cmd,
compiler_resource_dir):
"""
GCC and Clang handles the certain compiler intrinsics for specific
instruction sets (like SSE, AVX) differently. They have their own set of
Expand All @@ -95,6 +96,14 @@ def extend_analyzer_cmd_with_resource_dir(analyzer_cmd, compiler_resource_dir):
and describes our intentions more cleanly if we just simply disable the
inludes from the resource dir.
"""
if len(compiler_resource_dir) > 0:
analyzer_cmd.extend(['-nobuiltininc',
'-isystem', compiler_resource_dir])
if len(compiler_resource_dir) == 0:
return

resource_inc = compiler_resource_dir
# Resource include path must end with "/include".
basename = os.path.basename(os.path.normpath(resource_inc))
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe better to move this logic to __get_compiler_resource_dir?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is not resolved.

Copy link
Contributor Author

@martong martong Nov 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

On a long run we plan to use a function which takes both the system includes and the resource dir as a parameter, so we could implement just in one place the madness around this header include order.

if basename != 'include':
resource_inc = os.path.join(resource_inc, "include")

analyzer_cmd.extend(['-nobuiltininc',
'-isystem', resource_inc])
5 changes: 3 additions & 2 deletions libcodechecker/analyze/analyzers/analyzer_clang_tidy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,6 @@ def construct_analyzer_cmd(self, res_handler):

analyzer_cmd.append('-Qunused-arguments')

extend_analyzer_cmd_with_resource_dir(analyzer_cmd,
config.compiler_resource_dir)
# Set language.
analyzer_cmd.extend(['-x', self.buildaction.lang])

Expand All @@ -106,6 +104,9 @@ def construct_analyzer_cmd(self, res_handler):

analyzer_cmd.extend(self.buildaction.compiler_includes)

extend_analyzer_cmd_with_resource_dir(analyzer_cmd,
config.compiler_resource_dir)

return analyzer_cmd

except Exception as ex:
Expand Down
6 changes: 3 additions & 3 deletions libcodechecker/analyze/analyzers/analyzer_clangsa.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,9 +90,6 @@ def construct_analyzer_cmd(self, result_handler):

analyzer_cmd = [config.analyzer_binary]

extend_analyzer_cmd_with_resource_dir(analyzer_cmd,
config.compiler_resource_dir)

# Do not warn about the unused gcc/g++ arguments.
analyzer_cmd.append('-Qunused-arguments')

Expand Down Expand Up @@ -151,6 +148,9 @@ def construct_analyzer_cmd(self, result_handler):

analyzer_cmd.extend(self.buildaction.compiler_includes)

extend_analyzer_cmd_with_resource_dir(analyzer_cmd,
config.compiler_resource_dir)

analyzer_cmd.append(self.source_file)

return analyzer_cmd
Expand Down
35 changes: 33 additions & 2 deletions libcodechecker/analyze/analyzers/analyzer_types.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,19 @@ def replacer(matchobj):
return replacer


def __get_compiler_resource_dir(context, analyzer_binary):
resource_dir = ''
if len(context.compiler_resource_dir) > 0:
resource_dir = context.compiler_resource_dir
# If not set then ask the binary for the resource dir.
else:
# Can be None if Clang is too old.
resource_dir = host_check.get_resource_dir(analyzer_binary)
if resource_dir is None:
resource_dir = ""
return resource_dir


def __build_clangsa_config_handler(args, context):
"""
Build the config handler for clang static analyzer.
Expand All @@ -275,7 +288,8 @@ def __build_clangsa_config_handler(args, context):
config_handler = config_handler_clangsa.ClangSAConfigHandler()
config_handler.analyzer_plugins_dir = context.checker_plugin
config_handler.analyzer_binary = context.analyzer_binaries.get(CLANG_SA)
config_handler.compiler_resource_dir = context.compiler_resource_dir
config_handler.compiler_resource_dir =\
__get_compiler_resource_dir(context, config_handler.analyzer_binary)

if 'ctu_phases' in args:
config_handler.ctu_dir = os.path.join(args.output_path,
Expand Down Expand Up @@ -337,7 +351,24 @@ def __build_clang_tidy_config_handler(args, context):

config_handler = config_handler_clang_tidy.ClangTidyConfigHandler()
config_handler.analyzer_binary = context.analyzer_binaries.get(CLANG_TIDY)
config_handler.compiler_resource_dir = context.compiler_resource_dir

# FIXME We cannot get the resource dir from the clang-tidy binary,
# therefore now we get a clang binary which is a sibling of the clang-tidy.
# TODO Support "clang-tidy -print-resource-dir" .
check_env = analyzer_env.get_check_env(context.path_env_extra,
context.ld_lib_path_extra)
# Overwrite PATH to contain only the parent of the clang binary.
if os.path.isabs(config_handler.analyzer_binary):
check_env['PATH'] = os.path.dirname(config_handler.analyzer_binary)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it a good idea to overwrite the PATH? What is the motivation behind this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just extend it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or in case the point is to always resolve to the absolute clang binary, maybe we could just set the binary instead of invoking the function that looks up the binary in the path?

Copy link
Contributor Author

@martong martong Nov 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

The idea is to lookup clang from $PATH when it is a relative path. This is handled with the local object on the stack with the name check_env.
However, when clang-tidy is absolute we overwrite the PATH in check_env so we lookup only in the parent dir of clang-tidy.

It might be confusing to append to the path, because we may end up with /a/b/c/clang for /x/y/z/clang-tidy. This is true nevertheless in the relative case too, but that we cannot work around.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think if we want tidy and clang from the same directory we should modify the lookup function.
Overwriting the path might have negative consequences when we use this environment. (E.g.: we can not even issue a cd command if we pass this environment to a Popen or similar functions.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Simply setting the binary will miss the binary e.g. clang-4.0, this the reason to use resolve_missing_binary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't write over the effective PATH in the global env. We use a local variable and we overwrite the path in that local variable.

clang_bin = analyzer_clangsa.ClangSA.resolve_missing_binary('clang',
check_env)
if os.path.isfile(clang_bin):
config_handler.compiler_resource_dir =\
__get_compiler_resource_dir(context, clang_bin)
else:
config_handler.compiler_resource_dir =\
__get_compiler_resource_dir(context,
config_handler.analyzer_binary)

try:
with open(args.tidy_args_cfg_file, 'rb') as tidy_cfg:
Expand Down
2 changes: 1 addition & 1 deletion libcodechecker/analyze/analyzers/ctu_triple_arch.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@ def get_compile_command(action, config, source='', output=''):
for other operations. """

cmd = [config.analyzer_binary]
cmd.extend(action.compiler_includes)
extend_analyzer_cmd_with_resource_dir(cmd,
config.compiler_resource_dir)
cmd.extend(action.compiler_includes)
cmd.append('-c')
cmd.extend(['-x', action.lang])
cmd.append(config.analyzer_extra_arguments)
Expand Down
26 changes: 26 additions & 0 deletions libcodechecker/analyze/host_check.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,3 +59,29 @@ def has_analyzer_feature(clang_bin, feature):
except OSError:
LOG.error('Failed to run: "' + ' '.join(cmd) + '"')
raise


def get_resource_dir(clang_bin):
"""
Returns the resource_dir of Clang or None if the switch is not supported by
Clang.
"""
cmd = [clang_bin, "-print-resource-dir"]
LOG.debug('run: "' + ' '.join(cmd) + '"')
try:
proc = subprocess.Popen(cmd,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
)
out, err = proc.communicate()

LOG.debug("stdout:\n" + out)
LOG.debug("stderr:\n" + err)

if proc.returncode == 0:
return out.decode("utf-8").rstrip()
else:
return None
except OSError:
LOG.error('Failed to run: "' + ' '.join(cmd) + '"')
raise