r342827 - Fix modules build with shared library.

Tue Nov 6 09:52:34 PST 2018

Shuai - have you had a chance to look at this?

On Mon, Oct 22, 2018 at 4:43 PM Richard Smith <richard at metafoo.co.uk> wrote:

> On Mon, 22 Oct 2018 at 14:57, David Blaikie via cfe-commits <
> cfe-commits at lists.llvm.org> wrote:
>
>> Richard - any further thoughts here (re: layering/dependencies, etc)?
>> Would love to get the layering oddity fixed rather than having it get more
>> embedded over time.
>>
>
> Here's the intended current directory layout and layering as I understand
> it:
>
>  * include/clang/Analysis and lib/Analysis are the parts of the static
> analysis engine that are depended on by both Sema and StaticAnalyzer, and
> include common functionality / infrastructure
>  * include/clang/Analysis/Analyses and lib/Analysis/Analyses are the
> specific Sema analyses (that don't depend on the static analyzer);
> StaticAnalyzer should not need to depend on this
>  * the StaticAnalyzer library should contain all the pieces that are
> specific to the static analyzer
>  * Sema should not depend on Tooling, but it's fine for StaticAnalyzer to
> depend on Tooling
>
> Compared to where we are today, there are two differences:
>
> 1) The implementations of the headers in include/clang/Analysis are in
> lib/Analysis, not in lib/Analysis/Analyses
> 2) ExprMutationAnalyzer is in lib/Analysis despite not being part of the
> common infrastructure depended on by Sema and StaticAnalyzer
>
> For point 1, we should change the lib/Analysis directory layout to match
> the headers.
> For point 2, we should find a home for this ExprMutationAnalyzer code that
> makes sense. Given that the intent is to use it from the static analyzer,
> that seems like a reasonable home for it, but libTooling would also make
> some sense (perhaps a new subdirectory Tooling/Analysis), since it's only
> performing AST matching, not a flow-sensitive / path-sensitive static
> analysis.
>
> On Tue, Oct 2, 2018 at 2:44 PM Shuai Wang via cfe-commits <
>> cfe-commits at lists.llvm.org> wrote:
>>
>>> On Mon, Oct 1, 2018 at 4:58 PM Richard Smith <richard at metafoo.co.uk>
>>> wrote:
>>>
>>>> On Mon, 1 Oct 2018 at 16:10, George Karpenkov via cfe-commits <
>>>> cfe-commits at lists.llvm.org> wrote:
>>>>
>>>>> Hi Richard,
>>>>>
>>>>> On Oct 1, 2018, at 2:50 PM, Richard Smith via cfe-commits <
>>>>> cfe-commits at lists.llvm.org> wrote:
>>>>>
>>>>> This looks like the wrong fix to me, but I don't really know enough
>>>>> about what's being done with ExprMutationAnalyzer to have an opinion on
>>>>> what the right fix is.
>>>>>
>>>>> Shuai, what is the goal here? Why is this code being moved to
>>>>> Analysis/?
>>>>>
>>>>>
>>>>> I’ve asked for this possibility, as I wanted to use it from the Clang
>>>>> static analyzer.
>>>>>
>>>>> Do you intend to call it from the compiler frontend at some point? I
>>>>> can see a code review for the move itself, but no discussion of a plan or
>>>>> overall direction being followed here. Did I miss it?
>>>>>
>>>>> We have historically decided to not use the tooling interfaces
>>>>> (ASTMatcher, ParentMap, etc) from the frontend,
>>>>>
>>>>>
>>>>> Clang analyzer uses ASTMatcher all over the place.
>>>>>
>>>>> because they're generally a poor fit for our goals (we aim for a
>>>>> largely-single-pass compilation with good locality, and the AST matchers
>>>>> make different design choices)
>>>>>
>>>>>
>>>>> That’s totally good for the analyzer though, right?
>>>>>
>>>>
>>>> Yes, that all seems fine for the static analyzer.
>>>>
>>>>
>>>>> In any case, in future it might make sense to move the analyzer out of
>>>>> Clang proper.
>>>>> But for know the only way to use clang-tidy utilities from the
>>>>> analyzer is to move them into Clang.
>>>>>
>>>>
>>>> Right. I think we should try to maintain the idea that all the Clang
>>>> Static Analyzer-specific things are in lib/StaticAnalyzer, and lib/Analysis
>>>> only contains things depended on by the frontend. (That is: the things a
>>>> clang binary would use if we didn't link in the static analyzer)
>>>>
>>>> Given the above, I think the best approach would be to move this code
>>>> out of lib/Analysis and into somewhere in lib/StaticAnalyzer. There
>>>> shouldn't be any problem with clang-tidy using it from there, since it
>>>> already depends on the static analyzer.
>>>>
>>> I'm happy to do the move.
>>> Could you (or someone) help point out where exactly I should move it to
>>> though?
>>>
>>>> . If you want to change that, we'll need to discuss that decision.
>>>>>
>>>>> If the idea is to move this code into clang proper so that it can be
>>>>> used by various different tooling clients, we'd need to discuss the right
>>>>> place for it. Perhaps lib/Tooling/Analysis would make sense?
>>>>>
>>>>>
>>>>> On Mon, 1 Oct 2018 at 13:13, David Blaikie via cfe-commits <
>>>>> cfe-commits at lists.llvm.org> wrote:
>>>>>
>>>>>> I can't really reproduce this - when I try to build clang/llvm with
>>>>>> LLVM_ENABLE_MODULES in CMake I'm still seeing an error I reported March on
>>>>>> a cfe-dev thread - something to do with unique_ptr instantiations for
>>>>>> MappedBlockStream in the PDB parsing code.
>>>>>>
>>>>>> So, I'm wondering what error you hit, Eric/where/how, etc...
>>>>>>
>>>>>> On Sun, Sep 30, 2018 at 10:23 AM Eric Fiselier <eric at efcs.ca> wrote:
>>>>>>
>>>>>>> +rsmith (actually this time)
>>>>>>>
>>>>>>> On Sun, Sep 30, 2018 at 12:09 PM Eric Fiselier <eric at efcs.ca> wrote:
>>>>>>>
>>>>>>>> +rsmith
>>>>>>>>
>>>>>>>> Hi All,
>>>>>>>>
>>>>>>>> Sorry, I'm not actually sure why this fix is correct.I stole both
>>>>>>>> the fix and the comment from a similar one on L150 of the module map left
>>>>>>>> by Richard Smith.
>>>>>>>>
>>>>>>>> /Eric
>>>>>>>>
>>>>>>>> On Tue, Sep 25, 2018 at 8:36 PM Shuai Wang <shuaiwang at google.com>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>>> I'd like to understand this better as well, in particular what
>>>>>>>>> would be a proper fix?
>>>>>>>>>
>>>>>>>>> On Tue, Sep 25, 2018 at 2:15 PM David Blaikie <dblaikie at gmail.com>
>>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>> +Shuai Wang
>>>>>>>>>>
>>>>>>>>>> On Tue, Sep 25, 2018 at 2:14 PM David Blaikie <dblaikie at gmail.com>
>>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>>> Hey Eric - thanks for the fix - but could you explain the issue
>>>>>>>>>>> here in a bit more detail, as I'm a bit confused (& really interested in
>>>>>>>>>>> understanding any layering problems in LLVM - and fixing them/making sure
>>>>>>>>>>> they're fixed/holding the line/etc)
>>>>>>>>>>>
>>>>>>>>>>> What do you mean by "pull all of the AST matchers library into
>>>>>>>>>>> clang" - how does including a header ever add a link dependency?
>>>>>>>>>>>
>>>>>>>>>>> - Dave
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On Sat, Sep 22, 2018 at 5:49 PM Eric Fiselier via cfe-commits <
>>>>>>>>>>> cfe-commits at lists.llvm.org> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> Author: ericwf
>>>>>>>>>>>> Date: Sat Sep 22 17:48:05 2018
>>>>>>>>>>>> New Revision: 342827
>>>>>>>>>>>>
>>>>>>>>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=342827&view=rev
>>>>>>>>>>>> Log:
>>>>>>>>>>>> Fix modules build with shared library.
>>>>>>>>>>>>
>>>>>>>>>>>> r341994 caused clangAnalysis to pull all of the AST matchers
>>>>>>>>>>>> library into clang. Due to inline key functions in the headers,
>>>>>>>>>>>> importing the AST matchers library gives a link dependency on
>>>>>>>>>>>> the
>>>>>>>>>>>> AST matchers (and thus the AST), which clang should not
>>>>>>>>>>>> have.
>>>>>>>>>>>>
>>>>>>>>>>>> This patch works around the issues by excluding the offending
>>>>>>>>>>>> libclangAnalysis header in the modulemap.
>>>>>>>>>>>>
>>>>>>>>>>>> Modified:
>>>>>>>>>>>>     cfe/trunk/include/clang/module.modulemap
>>>>>>>>>>>>
>>>>>>>>>>>> Modified: cfe/trunk/include/clang/module.modulemap
>>>>>>>>>>>> URL:
>>>>>>>>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/module.modulemap?rev=342827&r1=342826&r2=342827&view=diff
>>>>>>>>>>>>
>>>>>>>>>>>> ==============================================================================
>>>>>>>>>>>> --- cfe/trunk/include/clang/module.modulemap (original)
>>>>>>>>>>>> +++ cfe/trunk/include/clang/module.modulemap Sat Sep 22
>>>>>>>>>>>> 17:48:05 2018
>>>>>>>>>>>> @@ -5,6 +5,12 @@ module Clang_Analysis {
>>>>>>>>>>>>    textual header "Analysis/Analyses/ThreadSafetyOps.def"
>>>>>>>>>>>>
>>>>>>>>>>>>    module * { export * }
>>>>>>>>>>>> +
>>>>>>>>>>>> +  // FIXME: Exclude these headers to avoid pulling all of the
>>>>>>>>>>>> AST matchers
>>>>>>>>>>>> +  // library into clang. Due to inline key functions in the
>>>>>>>>>>>> headers,
>>>>>>>>>>>> +  // importing the AST matchers library gives a link
>>>>>>>>>>>> dependency on the AST
>>>>>>>>>>>> +  // matchers (and thus the AST), which clang-format should
>>>>>>>>>>>> not have.
>>>>>>>>>>>> +  exclude header "Analysis/Analyses/ExprMutationAnalyzer.h"
>>>>>>>>>>>>  }
>>>>>>>>>>>>
>>>>>>>>>>>>  module Clang_AST {
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> _______________________________________________
>>>>>>>>>>>> cfe-commits mailing list
>>>>>>>>>>>> cfe-commits at lists.llvm.org
>>>>>>>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>>>>>>>>>
>>>>>>>>>>> _______________________________________________
>>>>>> cfe-commits mailing list
>>>>>> cfe-commits at lists.llvm.org
>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>>>
>>>>> _______________________________________________
>>>>> cfe-commits mailing list
>>>>> cfe-commits at lists.llvm.org
>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> cfe-commits mailing list
>>>>> cfe-commits at lists.llvm.org
>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>>
>>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20181106/4a3b9e70/attachment-0001.html>