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

Use arch of the analizer machine instead of the original one. #1308

Merged
merged 1 commit into from
Jan 24, 2018

Conversation

bruntib
Copy link
Contributor

@bruntib bruntib commented Jan 22, 2018

When creating the content of the ctu-dir the target architecture was
used to which compilation happened. If the analysis runs on a different
architecture (e.g. for debugging reasons) then the analyzer can't find
the necessary files of which the paths are named after the architecture.

@bruntib bruntib requested a review from gerazo January 22, 2018 13:59
@gerazo
Copy link

gerazo commented Jan 22, 2018

It is by design that we use the target architecture since this is the only way to have multiple directories for multiple analysis on the same machine if the code is cross-compiled to different architectures at once. This is why it is important to get this arch setting from clang itself instead of relying on the current machine we are running on. The ctu-dir is supposed to have temporary stuff which is only good for the same clang arch and version. If you use these internals in a different version of clang (as I understand, you want to use an other clang on a different platform), there is no guarantee that things don't break.

@gerazo
Copy link

gerazo commented Jan 22, 2018

(...and I'm afraid we don't have tests for cross-compiled, multiple-arch-in-one-build projects, so be aware!)

@bruntib
Copy link
Contributor Author

bruntib commented Jan 22, 2018

I thought it had a reason. But the other option fixing this would be a fix in the Clang code base where this externalFnMap is read.

@whisperity
Copy link
Contributor

So what is the issue itself? Clang tries to read the externalFnMap from its own triple path instead of using the triple generated from the command that is getting analysed?

@whisperity whisperity added the analyzer 📈 Related to the analyze commands (analysis driver) label Jan 22, 2018
@bruntib bruntib added the WIP 💣 Work In Progress label Jan 22, 2018
When creating the content of the ctu-dir the target architecture was
used to which compilation happened. If the analysis runs on a different
architecture (e.g. for debugging reasons) then the analyzer can't find
the necessary files of which the paths are named after the architecture.
@bruntib
Copy link
Contributor Author

bruntib commented Jan 22, 2018

OK, other solution uploaded.

Copy link

@gerazo gerazo left a comment

Choose a reason for hiding this comment

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

I like this solution now. I think it addresses the problem at the right place. I'm not sure where you need the fallback to platform.machine() but anyway it is proper for testing.

@bruntib bruntib removed the WIP 💣 Work In Progress label Jan 22, 2018
@whisperity whisperity added dev env ⛑️ Development environment and removed analyzer 📈 Related to the analyze commands (analysis driver) labels Jan 22, 2018
if flag.startswith('--target='):
return flag[9:].split('-')[0] # 9 == len('--target=')

return platform.machine()
Copy link
Contributor

@martong martong Jan 23, 2018

Choose a reason for hiding this comment

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

Would platform.machine() return the same string which clang uses when --target is not given?

Copy link
Contributor

Choose a reason for hiding this comment

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

Debatable. What happens if the architecture of Python and Clang differs, exactly one of them is 32-bit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just a fallback in case there is no --target given in the clang command invocation. This platform.machine() returns the architecture of the current machine where this Python script is run. If it differs from the one on which the build action was run, then it still doesn't work, like before this patch.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, let's merge it now because we need this badly and most often --target is given.

@martong
Copy link
Contributor

martong commented Jan 23, 2018

@gerazo Recently we changed CTU code in Clang to not allow merging ASTs from different targets.
https://github.com/Ericsson/clang/pull/244/files

@martong martong merged commit 48e4d04 into Ericsson:master Jan 24, 2018
@gyorb gyorb added this to the release 6.5 milestone Jan 24, 2018
@bruntib bruntib deleted the arch_triple_fix branch January 25, 2018 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev env ⛑️ Development environment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants