r342827 - Fix modules build with shared library.

Mon Oct 22 14:57:18 PDT 2018

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.

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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20181022/904ca139/attachment.html>