Cross Translation Unit Support in Clang

classic Classic list List threaded Threaded
26 messages Options
12
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Cross Translation Unit Support in Clang

Sumner, Brian via cfe-dev
Hi!

It looks like there is a consensus to accept the cross translation unit analysis patch into the clang static analyzer: https://reviews.llvm.org/D30691

There is one part of the patch that is independent of the static analyzer. The logic which can look up a function in an index and load a serialized AST that contains the definition of the function.
The lookup is cached, and after the AST is loaded, the function definition will be merged into the original AST.

Right now, in the current patch, this functionality is in the ASTContext. This is definitely not the right place to put this. So the question is, do you plan to utilize similar functionality in Clang tooling or clang tidy?

If yes, we might end up creating a new module for that (or add it to an existing commonly used one like libtooling?). If no, we will move the corresponding code to the static analyzer.

What do you think? 

In case you are interested in how it works, you can check out the EuroLLVM talk: http://llvm.org/devmtg/2017-03//2017/02/20/accepted-sessions.html#7

Regards,
Gábor

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Cross Translation Unit Support in Clang

Sumner, Brian via cfe-dev
Hello Gabor,

Internally, we have created XTU module inside clang (lib/XTU). I think
it is the best way because importing is not related to analyzer
directly. We're not going to use it outside of CSA but I think future
users should have such possibility.

22.06.2017 12:41, Gábor Horváth пишет:

> Hi!
>
> It looks like there is a consensus to accept the cross translation
> unit analysis patch into the clang static analyzer:
> https://reviews.llvm.org/D30691
>
> There is one part of the patch that is independent of the static
> analyzer. The logic which can look up a function in an index and load
> a serialized AST that contains the definition of the function.
> The lookup is cached, and after the AST is loaded, the function
> definition will be merged into the original AST.
>
> Right now, in the current patch, this functionality is in the
> ASTContext. This is definitely not the right place to put this. So the
> question is, do you plan to utilize similar functionality in Clang
> tooling or clang tidy?
>
> If yes, we might end up creating a new module for that (or add it to
> an existing commonly used one like libtooling?). If no, we will move
> the corresponding code to the static analyzer.
>
> What do you think?
>
> In case you are interested in how it works, you can check out the
> EuroLLVM talk:
> http://llvm.org/devmtg/2017-03//2017/02/20/accepted-sessions.html#7
>
> Regards,
> Gábor


--
Best regards,
Aleksei Sidorin,
SRR, Samsung Electronics

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Cross Translation Unit Support in Clang

Sumner, Brian via cfe-dev
For clang tooling, I don't really expect us to do cross-TU AST loading outside of what modules will provide, as that inherently doesn't scale. Instead, we usually design pipelined approaches where the first "scan" over the codebase provides data which are reduced to the information needed for the tool.

That said, I can't predict the future, and people have expressed interest in that functionality, so
a) I agree it's a good idea to add and 
b) I think it's a good fit for libtooling

Cheers,
/Manuel


On Thu, Jun 22, 2017 at 11:58 AM Aleksei Sidorin <[hidden email]> wrote:
Hello Gabor,

Internally, we have created XTU module inside clang (lib/XTU). I think
it is the best way because importing is not related to analyzer
directly. We're not going to use it outside of CSA but I think future
users should have such possibility.

22.06.2017 12:41, Gábor Horváth пишет:
> Hi!
>
> It looks like there is a consensus to accept the cross translation
> unit analysis patch into the clang static analyzer:
> https://reviews.llvm.org/D30691
>
> There is one part of the patch that is independent of the static
> analyzer. The logic which can look up a function in an index and load
> a serialized AST that contains the definition of the function.
> The lookup is cached, and after the AST is loaded, the function
> definition will be merged into the original AST.
>
> Right now, in the current patch, this functionality is in the
> ASTContext. This is definitely not the right place to put this. So the
> question is, do you plan to utilize similar functionality in Clang
> tooling or clang tidy?
>
> If yes, we might end up creating a new module for that (or add it to
> an existing commonly used one like libtooling?). If no, we will move
> the corresponding code to the static analyzer.
>
> What do you think?
>
> In case you are interested in how it works, you can check out the
> EuroLLVM talk:
> http://llvm.org/devmtg/2017-03//2017/02/20/accepted-sessions.html#7
>
> Regards,
> Gábor


--
Best regards,
Aleksei Sidorin,
SRR, Samsung Electronics


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Cross Translation Unit Support in Clang

Sumner, Brian via cfe-dev
Anna,

Are you ok having libTooling as a dependency of the Static Analyzer? I think this is not a bad direction since it has other good utilities that the Static Analyzer could use in the future such as Replacements, FixIts.

Regards,
Gábor

On 22 June 2017 at 12:10, Manuel Klimek <[hidden email]> wrote:
For clang tooling, I don't really expect us to do cross-TU AST loading outside of what modules will provide, as that inherently doesn't scale. Instead, we usually design pipelined approaches where the first "scan" over the codebase provides data which are reduced to the information needed for the tool.

That said, I can't predict the future, and people have expressed interest in that functionality, so
a) I agree it's a good idea to add and 
b) I think it's a good fit for libtooling

Cheers,
/Manuel


On Thu, Jun 22, 2017 at 11:58 AM Aleksei Sidorin <[hidden email]> wrote:
Hello Gabor,

Internally, we have created XTU module inside clang (lib/XTU). I think
it is the best way because importing is not related to analyzer
directly. We're not going to use it outside of CSA but I think future
users should have such possibility.

22.06.2017 12:41, Gábor Horváth пишет:
> Hi!
>
> It looks like there is a consensus to accept the cross translation
> unit analysis patch into the clang static analyzer:
> https://reviews.llvm.org/D30691
>
> There is one part of the patch that is independent of the static
> analyzer. The logic which can look up a function in an index and load
> a serialized AST that contains the definition of the function.
> The lookup is cached, and after the AST is loaded, the function
> definition will be merged into the original AST.
>
> Right now, in the current patch, this functionality is in the
> ASTContext. This is definitely not the right place to put this. So the
> question is, do you plan to utilize similar functionality in Clang
> tooling or clang tidy?
>
> If yes, we might end up creating a new module for that (or add it to
> an existing commonly used one like libtooling?). If no, we will move
> the corresponding code to the static analyzer.
>
> What do you think?
>
> In case you are interested in how it works, you can check out the
> EuroLLVM talk:
> http://llvm.org/devmtg/2017-03//2017/02/20/accepted-sessions.html#7
>
> Regards,
> Gábor


--
Best regards,
Aleksei Sidorin,
SRR, Samsung Electronics



_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Cross Translation Unit Support in Clang

Sumner, Brian via cfe-dev

On Jun 23, 2017, at 1:40 AM, Gábor Horváth <[hidden email]> wrote:

Anna,

Are you ok having libTooling as a dependency of the Static Analyzer?

Are we talking about introducing dependency for scan-build or clang itself?

I think this is not a bad direction since it has other good utilities that the Static Analyzer could use in the future such as Replacements, FixIts.

Regards,
Gábor

On 22 June 2017 at 12:10, Manuel Klimek <[hidden email]> wrote:
For clang tooling, I don't really expect us to do cross-TU AST loading outside of what modules will provide, as that inherently doesn't scale. Instead, we usually design pipelined approaches where the first "scan" over the codebase provides data which are reduced to the information needed for the tool.

That said, I can't predict the future, and people have expressed interest in that functionality, so
a) I agree it's a good idea to add and 
b) I think it's a good fit for libtooling

Cheers,
/Manuel


On Thu, Jun 22, 2017 at 11:58 AM Aleksei Sidorin <[hidden email]> wrote:
Hello Gabor,

Internally, we have created XTU module inside clang (lib/XTU). I think
it is the best way because importing is not related to analyzer
directly. We're not going to use it outside of CSA but I think future
users should have such possibility.

22.06.2017 12:41, Gábor Horváth пишет:
> Hi!
>
> It looks like there is a consensus to accept the cross translation
> unit analysis patch into the clang static analyzer:
> https://reviews.llvm.org/D30691
>
> There is one part of the patch that is independent of the static
> analyzer. The logic which can look up a function in an index and load
> a serialized AST that contains the definition of the function.
> The lookup is cached, and after the AST is loaded, the function
> definition will be merged into the original AST.
>
> Right now, in the current patch, this functionality is in the
> ASTContext. This is definitely not the right place to put this. So the
> question is, do you plan to utilize similar functionality in Clang
> tooling or clang tidy?
>
> If yes, we might end up creating a new module for that (or add it to
> an existing commonly used one like libtooling?). If no, we will move
> the corresponding code to the static analyzer.
>
> What do you think?
>
> In case you are interested in how it works, you can check out the
> EuroLLVM talk:
> http://llvm.org/devmtg/2017-03//2017/02/20/accepted-sessions.html#7
>
> Regards,
> Gábor


--
Best regards,
Aleksei Sidorin,
SRR, Samsung Electronics




_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Cross Translation Unit Support in Clang

Sumner, Brian via cfe-dev
This is a dependency for the Static Analyzer component within clang (to another component, which is in clang as well). It is a similar dependency to this: https://github.com/llvm-mirror/clang/commit/a994aad333a56b8c9dd49bcfb5090a393d193387

On 23 June 2017 at 19:48, Anna Zaks <[hidden email]> wrote:

On Jun 23, 2017, at 1:40 AM, Gábor Horváth <[hidden email]> wrote:

Anna,

Are you ok having libTooling as a dependency of the Static Analyzer?

Are we talking about introducing dependency for scan-build or clang itself?

I think this is not a bad direction since it has other good utilities that the Static Analyzer could use in the future such as Replacements, FixIts.

Regards,
Gábor

On 22 June 2017 at 12:10, Manuel Klimek <[hidden email]> wrote:
For clang tooling, I don't really expect us to do cross-TU AST loading outside of what modules will provide, as that inherently doesn't scale. Instead, we usually design pipelined approaches where the first "scan" over the codebase provides data which are reduced to the information needed for the tool.

That said, I can't predict the future, and people have expressed interest in that functionality, so
a) I agree it's a good idea to add and 
b) I think it's a good fit for libtooling

Cheers,
/Manuel


On Thu, Jun 22, 2017 at 11:58 AM Aleksei Sidorin <[hidden email]> wrote:
Hello Gabor,

Internally, we have created XTU module inside clang (lib/XTU). I think
it is the best way because importing is not related to analyzer
directly. We're not going to use it outside of CSA but I think future
users should have such possibility.

22.06.2017 12:41, Gábor Horváth пишет:
> Hi!
>
> It looks like there is a consensus to accept the cross translation
> unit analysis patch into the clang static analyzer:
> https://reviews.llvm.org/D30691
>
> There is one part of the patch that is independent of the static
> analyzer. The logic which can look up a function in an index and load
> a serialized AST that contains the definition of the function.
> The lookup is cached, and after the AST is loaded, the function
> definition will be merged into the original AST.
>
> Right now, in the current patch, this functionality is in the
> ASTContext. This is definitely not the right place to put this. So the
> question is, do you plan to utilize similar functionality in Clang
> tooling or clang tidy?
>
> If yes, we might end up creating a new module for that (or add it to
> an existing commonly used one like libtooling?). If no, we will move
> the corresponding code to the static analyzer.
>
> What do you think?
>
> In case you are interested in how it works, you can check out the
> EuroLLVM talk:
> http://llvm.org/devmtg/2017-03//2017/02/20/accepted-sessions.html#7
>
> Regards,
> Gábor


--
Best regards,
Aleksei Sidorin,
SRR, Samsung Electronics





_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Cross Translation Unit Support in Clang

Sumner, Brian via cfe-dev

On Jun 23, 2017, at 12:39 PM, Gábor Horváth <[hidden email]> wrote:

This is a dependency for the Static Analyzer component within clang (to another component, which is in clang as well).

OK. This means that we are talking about adding a dependency on libTooling to clang. This would not only effect the static analyzer, so we’d need an OK from a greater clang community.


There are 2 differences from the dependency on ASTMatchers:
 - Nothing that is presently in libTooling us used by clang.
 - The new component that the analyzer will depend on will be supporting a very experimental feature.

The other expedient options that I can see are:
 - Adding a separate library with just the functionality that this feature needs.
 - Making the dependency conditional, following the same style as Z3 support, and keeping it that way until the feature is fully implemented. This solution definitely has downsides.

Yet another solution is to pull the analyzer out of clang, but unfortunately that is non-trivial, so I am not sure if there are volunteers for the task.

Anna.

On 23 June 2017 at 19:48, Anna Zaks <[hidden email]> wrote:

On Jun 23, 2017, at 1:40 AM, Gábor Horváth <[hidden email]> wrote:

Anna,

Are you ok having libTooling as a dependency of the Static Analyzer?

Are we talking about introducing dependency for scan-build or clang itself?

I think this is not a bad direction since it has other good utilities that the Static Analyzer could use in the future such as Replacements, FixIts.

Regards,
Gábor

On 22 June 2017 at 12:10, Manuel Klimek <[hidden email]> wrote:
For clang tooling, I don't really expect us to do cross-TU AST loading outside of what modules will provide, as that inherently doesn't scale. Instead, we usually design pipelined approaches where the first "scan" over the codebase provides data which are reduced to the information needed for the tool.

That said, I can't predict the future, and people have expressed interest in that functionality, so
a) I agree it's a good idea to add and 
b) I think it's a good fit for libtooling

Cheers,
/Manuel


On Thu, Jun 22, 2017 at 11:58 AM Aleksei Sidorin <[hidden email]> wrote:
Hello Gabor,

Internally, we have created XTU module inside clang (lib/XTU). I think
it is the best way because importing is not related to analyzer
directly. We're not going to use it outside of CSA but I think future
users should have such possibility.

22.06.2017 12:41, Gábor Horváth пишет:
> Hi!
>
> It looks like there is a consensus to accept the cross translation
> unit analysis patch into the clang static analyzer:
> https://reviews.llvm.org/D30691
>
> There is one part of the patch that is independent of the static
> analyzer. The logic which can look up a function in an index and load
> a serialized AST that contains the definition of the function.
> The lookup is cached, and after the AST is loaded, the function
> definition will be merged into the original AST.
>
> Right now, in the current patch, this functionality is in the
> ASTContext. This is definitely not the right place to put this. So the
> question is, do you plan to utilize similar functionality in Clang
> tooling or clang tidy?
>
> If yes, we might end up creating a new module for that (or add it to
> an existing commonly used one like libtooling?). If no, we will move
> the corresponding code to the static analyzer.
>
> What do you think?
>
> In case you are interested in how it works, you can check out the
> EuroLLVM talk:
> http://llvm.org/devmtg/2017-03//2017/02/20/accepted-sessions.html#7
>
> Regards,
> Gábor


--
Best regards,
Aleksei Sidorin,
SRR, Samsung Electronics






_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Cross Translation Unit Support in Clang

Sumner, Brian via cfe-dev
Hi,

Some benchmarks of the binary size after introducing the libTooling dependency to clang (using static linking on Linux):

Release:
85457072 -> 85505864
Debug:
1777932672 -> 1779938696

The increase is less than 1% in both cases. So, in my opinion, the binary size is not really a problem here.
Of course, the link times might increase a bit as well.

A question is, who should make the call whether this penalty is ok or not?

On 23 June 2017 at 22:06, Anna Zaks <[hidden email]> wrote:

On Jun 23, 2017, at 12:39 PM, Gábor Horváth <[hidden email]> wrote:

This is a dependency for the Static Analyzer component within clang (to another component, which is in clang as well).

OK. This means that we are talking about adding a dependency on libTooling to clang. This would not only effect the static analyzer, so we’d need an OK from a greater clang community.


There are 2 differences from the dependency on ASTMatchers:
 - Nothing that is presently in libTooling us used by clang.

This is actually not a difference. When the commit above was excepted, it was the first use of ASTMatchers in the clang binary.
 
 - The new component that the analyzer will depend on will be supporting a very experimental feature.

The other expedient options that I can see are:
 - Adding a separate library with just the functionality that this feature needs.
 - Making the dependency conditional, following the same style as Z3 support, and keeping it that way until the feature is fully implemented. This solution definitely has downsides.

Yet another solution is to pull the analyzer out of clang, but unfortunately that is non-trivial, so I am not sure if there are volunteers for the task.

I agree that pulling the analyzer out is a big task and would also break the backward compatibility of the command line. So it is not the best solution for the users.

Regards,
Gábor
 

Anna.

On 23 June 2017 at 19:48, Anna Zaks <[hidden email]> wrote:

On Jun 23, 2017, at 1:40 AM, Gábor Horváth <[hidden email]> wrote:

Anna,

Are you ok having libTooling as a dependency of the Static Analyzer?

Are we talking about introducing dependency for scan-build or clang itself?

I think this is not a bad direction since it has other good utilities that the Static Analyzer could use in the future such as Replacements, FixIts.

Regards,
Gábor

On 22 June 2017 at 12:10, Manuel Klimek <[hidden email]> wrote:
For clang tooling, I don't really expect us to do cross-TU AST loading outside of what modules will provide, as that inherently doesn't scale. Instead, we usually design pipelined approaches where the first "scan" over the codebase provides data which are reduced to the information needed for the tool.

That said, I can't predict the future, and people have expressed interest in that functionality, so
a) I agree it's a good idea to add and 
b) I think it's a good fit for libtooling

Cheers,
/Manuel


On Thu, Jun 22, 2017 at 11:58 AM Aleksei Sidorin <[hidden email]> wrote:
Hello Gabor,

Internally, we have created XTU module inside clang (lib/XTU). I think
it is the best way because importing is not related to analyzer
directly. We're not going to use it outside of CSA but I think future
users should have such possibility.

22.06.2017 12:41, Gábor Horváth пишет:
> Hi!
>
> It looks like there is a consensus to accept the cross translation
> unit analysis patch into the clang static analyzer:
> https://reviews.llvm.org/D30691
>
> There is one part of the patch that is independent of the static
> analyzer. The logic which can look up a function in an index and load
> a serialized AST that contains the definition of the function.
> The lookup is cached, and after the AST is loaded, the function
> definition will be merged into the original AST.
>
> Right now, in the current patch, this functionality is in the
> ASTContext. This is definitely not the right place to put this. So the
> question is, do you plan to utilize similar functionality in Clang
> tooling or clang tidy?
>
> If yes, we might end up creating a new module for that (or add it to
> an existing commonly used one like libtooling?). If no, we will move
> the corresponding code to the static analyzer.
>
> What do you think?
>
> In case you are interested in how it works, you can check out the
> EuroLLVM talk:
> http://llvm.org/devmtg/2017-03//2017/02/20/accepted-sessions.html#7
>
> Regards,
> Gábor


--
Best regards,
Aleksei Sidorin,
SRR, Samsung Electronics







_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Cross Translation Unit Support in Clang

Sumner, Brian via cfe-dev
On Mon, Jun 26, 2017 at 4:08 PM Gábor Horváth via cfe-dev <[hidden email]> wrote:
Hi,

Some benchmarks of the binary size after introducing the libTooling dependency to clang (using static linking on Linux):

Release:
85457072 -> 85505864
Debug:
1777932672 -> 1779938696

The increase is less than 1% in both cases. So, in my opinion, the binary size is not really a problem here.
Of course, the link times might increase a bit as well.

A question is, who should make the call whether this penalty is ok or not?

(after replying on the patch without noticing that this thread is not part of the patch, here it goes again :) 
Richard (cc'ed) usually owns decisions around clang itself. Writing an email to cfe-dev with the numbers and wait for whether others have concerns would probably also be good (many will probably not continue reading this thread).


On 23 June 2017 at 22:06, Anna Zaks <[hidden email]> wrote:

On Jun 23, 2017, at 12:39 PM, Gábor Horváth <[hidden email]> wrote:

This is a dependency for the Static Analyzer component within clang (to another component, which is in clang as well).

OK. This means that we are talking about adding a dependency on libTooling to clang. This would not only effect the static analyzer, so we’d need an OK from a greater clang community.


There are 2 differences from the dependency on ASTMatchers:
 - Nothing that is presently in libTooling us used by clang.

This is actually not a difference. When the commit above was excepted, it was the first use of ASTMatchers in the clang binary.
 
 - The new component that the analyzer will depend on will be supporting a very experimental feature.

The other expedient options that I can see are:
 - Adding a separate library with just the functionality that this feature needs.
 - Making the dependency conditional, following the same style as Z3 support, and keeping it that way until the feature is fully implemented. This solution definitely has downsides.

Yet another solution is to pull the analyzer out of clang, but unfortunately that is non-trivial, so I am not sure if there are volunteers for the task.

I agree that pulling the analyzer out is a big task and would also break the backward compatibility of the command line. So it is not the best solution for the users.

Regards,
Gábor
 

Anna.

On 23 June 2017 at 19:48, Anna Zaks <[hidden email]> wrote:

On Jun 23, 2017, at 1:40 AM, Gábor Horváth <[hidden email]> wrote:

Anna,

Are you ok having libTooling as a dependency of the Static Analyzer?

Are we talking about introducing dependency for scan-build or clang itself?

I think this is not a bad direction since it has other good utilities that the Static Analyzer could use in the future such as Replacements, FixIts.

Regards,
Gábor

On 22 June 2017 at 12:10, Manuel Klimek <[hidden email]> wrote:
For clang tooling, I don't really expect us to do cross-TU AST loading outside of what modules will provide, as that inherently doesn't scale. Instead, we usually design pipelined approaches where the first "scan" over the codebase provides data which are reduced to the information needed for the tool.

That said, I can't predict the future, and people have expressed interest in that functionality, so
a) I agree it's a good idea to add and 
b) I think it's a good fit for libtooling

Cheers,
/Manuel


On Thu, Jun 22, 2017 at 11:58 AM Aleksei Sidorin <[hidden email]> wrote:
Hello Gabor,

Internally, we have created XTU module inside clang (lib/XTU). I think
it is the best way because importing is not related to analyzer
directly. We're not going to use it outside of CSA but I think future
users should have such possibility.

22.06.2017 12:41, Gábor Horváth пишет:
> Hi!
>
> It looks like there is a consensus to accept the cross translation
> unit analysis patch into the clang static analyzer:
> https://reviews.llvm.org/D30691
>
> There is one part of the patch that is independent of the static
> analyzer. The logic which can look up a function in an index and load
> a serialized AST that contains the definition of the function.
> The lookup is cached, and after the AST is loaded, the function
> definition will be merged into the original AST.
>
> Right now, in the current patch, this functionality is in the
> ASTContext. This is definitely not the right place to put this. So the
> question is, do you plan to utilize similar functionality in Clang
> tooling or clang tidy?
>
> If yes, we might end up creating a new module for that (or add it to
> an existing commonly used one like libtooling?). If no, we will move
> the corresponding code to the static analyzer.
>
> What do you think?
>
> In case you are interested in how it works, you can check out the
> EuroLLVM talk:
> http://llvm.org/devmtg/2017-03//2017/02/20/accepted-sessions.html#7
>
> Regards,
> Gábor


--
Best regards,
Aleksei Sidorin,
SRR, Samsung Electronics





_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Cross Translation Unit Support in Clang

Sumner, Brian via cfe-dev
In reply to this post by Sumner, Brian via cfe-dev
I think this post refers to the subject of the recent "Cross Translation Unit Support in Clang" thread, but I'm not entirely sure I understand the issue, so I'll start a new thread rather than potentially pollute that one.

So I've been working on a tool[1], based on libTooling, that automatically translates C code to SaferCPlusPlus (essentially, a memory-safe subset of C++). The problem I have, if I'm understanding it correctly, is that it can only convert one source file (which corresponds to a "translation unit" right?) at a time, due to libTooling's  limitation. Right? Does that make sense?

Ok, so converting a source file (potentially) modifies the file itself and any included header files as well. The problem is when multiple source files include the same header file, because conversion of a source file requires the included header files to be in their original form. They can't have been modified by a previous conversion of another source file. So after (individually) converting all the source files in a project, you can end up with multiple versions of a header file that were modified in different ways by the conversions of different source files. Right? (An example[2].)

So this means to get the final converted version of the header file you have to merge the modifications made by each conversion operation. And sometimes the modifications are made on the same line so the merge tool can't do the merge automatically. (At least meld doesn't.) This is really annoying.

Now, if libTooling were able to operate on the AST of the entire project at once this problem would go away. Or if you think the AST of the whole project would often be too big, then at least multiple specified translation units at a time would help.

I'm not sure if this is what's being offered in the "Cross Translation Unit Support in Clang" thread? I would want the Rewriter::overwriteChangedFiles() function to apply to all the source translation units of the AST as well.

Am I making make sense? Feel free to set me straight.

Noah

[1] https://github.com/duneroadrunner/SaferCPlusPlus-AutoTranslation
[2] https://github.com/duneroadrunner/SaferCPlusPlus-AutoTranslation/tree/master/examples/lodepng


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Cross Translation Unit Support in Clang

Sumner, Brian via cfe-dev
Hi Noah!

On 28 June 2017 at 19:48, Noah L via cfe-dev <[hidden email]> wrote:
I think this post refers to the subject of the recent "Cross Translation Unit Support in Clang" thread, but I'm not entirely sure I understand the issue, so I'll start a new thread rather than potentially pollute that one.

So I've been working on a tool[1], based on libTooling, that automatically translates C code to SaferCPlusPlus (essentially, a memory-safe subset of C++). The problem I have, if I'm understanding it correctly, is that it can only convert one source file (which corresponds to a "translation unit" right?) at a time, due to libTooling's  limitation. Right? Does that make sense?

That is correct. A translation unit is basically a file that is being compiled and all its dependencies, like headers, precompiled headers, modules.
 

Ok, so converting a source file (potentially) modifies the file itself and any included header files as well. The problem is when multiple source files include the same header file, because conversion of a source file requires the included header files to be in their original form. They can't have been modified by a previous conversion of another source file. So after (individually) converting all the source files in a project, you can end up with multiple versions of a header file that were modified in different ways by the conversions of different source files. Right? (An example[2].)

Well, as far as I understand, the idiomatic way to handle this right now, is not to apply your modifications to the source files right away. First export all the changes for each translation units into a separate file, and apply all the changes at once after the tool was run. If all your changes are consistent, that should work well. If you would transform a header in a different way in two different translation units that is a problem. I think the best way to solve that right now to make sure all of your edits are consistent.
 

So this means to get the final converted version of the header file you have to merge the modifications made by each conversion operation. And sometimes the modifications are made on the same line so the merge tool can't do the merge automatically. (At least meld doesn't.) This is really annoying.

Now, if libTooling were able to operate on the AST of the entire project at once this problem would go away. Or if you think the AST of the whole project would often be too big, then at least multiple specified translation units at a time would help.

This is not the usecase this functionality was designed for. I think you could definitely use it for something like that, but I think the solution I mentioned above is superior right now. The main use case we wanted to cover is the ability to gather some information from other translation units that are required for your tool.
 

I'm not sure if this is what's being offered in the "Cross Translation Unit Support in Clang" thread? I would want the Rewriter::overwriteChangedFiles() function to apply to all the source translation units of the AST as well.

Am I making make sense? Feel free to set me straight.

I hope I could help.

Regards,
Gábor
 

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev



_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Cross Translation Unit Support in Clang

Sumner, Brian via cfe-dev
(Oops, I meant to start a new thread :) Thanks for your response.

On Thu, Jun 29, 2017 at 12:01 AM, Gábor Horváth <[hidden email]> wrote:
...
 
Well, as far as I understand, the idiomatic way to handle this right now, is not to apply your modifications to the source files right away. First export all the changes for each translation units into a separate file, and apply all the changes at once after the tool was run. If all your changes are consistent, that should work well. If you would transform a header in a different way in two different translation units that is a problem. I think the best way to solve that right now to make sure all of your edits are consistent.

So then I couldn't use clang::Rewriter to apply the modifications. I would have to somehow obtain the line and column numbers of the clang::SourceRange to be replaced and do it manually? Is that how other people do it?

So, in my case the modifications are never contradictory, but each one may be insufficient in extent due to information not available in the translation unit. For example, one thing the tool does is modify the type declaration of a pointer depending on if a) it is being used as an array pointer/iterator, and b) if it is being used as a pointer to a dynamically sized array (i.e. basically if the array is, or could be, realloc()ed). You could imagine a situation where in one source file we see that a pointer target is realloc()ed, but we cannot determine whether or not the target is being used as an array (in which case no modification is made), and in another source file we can see that the target (of the same pointer) is (being used as) an array, but there is no indication of how the array was allocated (or realloc()ed). Without combining the observations from both source files, we cannot determine that the pointer target is a dynamically sized array (and hence won't be able to apply the (fully) correct modification).

And analyzing the modifications resulting from each source file processed individually is not enough either (since the first case will not result in a modification). So I would have to export more information than just the modifications determined from processing each file individually.

But in its most general form, that extra information is essentially the AST, no? (Or some subset of the AST that's relevant to me.) So basically you're suggesting that I export (some simplified version of) the ASTs to disk and combine them manually. And then analyze the combined (simplified) AST to come up with the correct modifications. Which I'd then have to apply manually.

I'm not saying it's not feasible. I'm not even saying it's not reasonable. But you can see why it'd be nicer for me if libTooling could just present me with a combined AST. :)

 
 

So this means to get the final converted version of the header file you have to merge the modifications made by each conversion operation. And sometimes the modifications are made on the same line so the merge tool can't do the merge automatically. (At least meld doesn't.) This is really annoying.

Now, if libTooling were able to operate on the AST of the entire project at once this problem would go away. Or if you think the AST of the whole project would often be too big, then at least multiple specified translation units at a time would help.

This is not the usecase this functionality was designed for. I think you could definitely use it for something like that, but I think the solution I mentioned above is superior right now. The main use case we wanted to cover is the ability to gather some information from other translation units that are required for your tool.

So, are you saying that the proposed changes would allow me, in a straightforward way, to obtain a (complete) AST spanning multiple translation units? (Where an element in a header file will have only one AST node even when the header file is included by multiple translation units?)

Noah


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Cross Translation Unit Support in Clang

Sumner, Brian via cfe-dev


On Thu, Jun 29, 2017, 9:23 PM Noah L via cfe-dev <[hidden email]> wrote:
(Oops, I meant to start a new thread :) Thanks for your response.

On Thu, Jun 29, 2017 at 12:01 AM, Gábor Horváth <[hidden email]> wrote:
...

 
Well, as far as I understand, the idiomatic way to handle this right now, is not to apply your modifications to the source files right away. First export all the changes for each translation units into a separate file, and apply all the changes at once after the tool was run. If all your changes are consistent, that should work well. If you would transform a header in a different way in two different translation units that is a problem. I think the best way to solve that right now to make sure all of your edits are consistent.

So then I couldn't use clang::Rewriter to apply the modifications. I would have to somehow obtain the line and column numbers of the clang::SourceRange to be replaced and do it manually? Is that how other people do it?

Yes. Libtooling replacements are decoupled from the source manager fit that reason and do that exact job.


So, in my case the modifications are never contradictory, but each one may be insufficient in extent due to information not available in the translation unit. For example, one thing the tool does is modify the type declaration of a pointer depending on if a) it is being used as an array pointer/iterator, and b) if it is being used as a pointer to a dynamically sized array (i.e. basically if the array is, or could be, realloc()ed). You could imagine a situation where in one source file we see that a pointer target is realloc()ed, but we cannot determine whether or not the target is being used as an array (in which case no modification is made), and in another source file we can see that the target (of the same pointer) is (being used as) an array, but there is no indication of how the array was allocated (or realloc()ed). Without combining the observations from both source files, we cannot determine that the pointer target is a dynamically sized array (and hence won't be able to apply the (fully) correct modification).

And analyzing the modifications resulting from each source file processed individually is not enough either (since the first case will not result in a modification). So I would have to export more information than just the modifications determined from processing each file individually.

But in its most general form, that extra information is essentially the AST, no? (Or some subset of the AST that's relevant to me.) So basically you're suggesting that I export (some simplified version of) the ASTs to disk and combine them manually. And then analyze the combined (simplified) AST to come up with the correct modifications. Which I'd then have to apply manually.

The idea is to export just the information you need, keyed on the thing you want to change. That way, you can fully parallelize both parsing and postprocessing in a large code base.


I'm not saying it's not feasible. I'm not even saying it's not reasonable. But you can see why it'd be nicer for me if libTooling could just present me with a combined AST. :)

Unfortunately that only scales to small projects in the generalized case. The static analyzer gets away with it because it only loads things close to the function it analyzes, instead of needing a global view.


 
 

So this means to get the final converted version of the header file you have to merge the modifications made by each conversion operation. And sometimes the modifications are made on the same line so the merge tool can't do the merge automatically. (At least meld doesn't.) This is really annoying.

Now, if libTooling were able to operate on the AST of the entire project at once this problem would go away. Or if you think the AST of the whole project would often be too big, then at least multiple specified translation units at a time would help.

This is not the usecase this functionality was designed for. I think you could definitely use it for something like that, but I think the solution I mentioned above is superior right now. The main use case we wanted to cover is the ability to gather some information from other translation units that are required for your tool.

So, are you saying that the proposed changes would allow me, in a straightforward way, to obtain a (complete) AST spanning multiple translation units? (Where an element in a header file will have only one AST node even when the header file is included by multiple translation units?)

Noah

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Cross Translation Unit Support in Clang

Sumner, Brian via cfe-dev
In reply to this post by Sumner, Brian via cfe-dev
On 2017-06-28 19:48, Noah L via cfe-dev wrote:

> Ok, so converting a source file (potentially) modifies the file itself
> and any included header files as well. The problem is when multiple
> source files include the same header file, because conversion of a
> source file requires the included header files to be in their original
> form. They can't have been modified by a previous conversion of another
> source file. So after (individually) converting all the source files in
> a project, you can end up with multiple versions of a header file that
> were modified in different ways by the conversions of different source
> files. Right? (An example[2].)

What if you only modify the file that was given to the tool? I.e. don't
modify the header files. Would that work?

In my tool I ignore everything that is not declared in the file given to
the tool [1].

You could also copy the files and do the transformation on the copies.
Might be a better idea anyway, to avoid overwriting the original files.

[1]
https://github.com/jacob-carlborg/dstep/blob/master/dstep/translator/Translator.d#L315

--
/Jacob Carlborg

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Cross Translation Unit Support in Clang

Sumner, Brian via cfe-dev
In reply to this post by Sumner, Brian via cfe-dev

On Thu, Jun 29, 2017 at 10:31 PM, Manuel Klimek <[hidden email]> wrote:
...

I'm not saying it's not feasible. I'm not even saying it's not reasonable. But you can see why it'd be nicer for me if libTooling could just present me with a combined AST. :)

Unfortunately that only scales to small projects in the generalized case. The static analyzer gets away with it because it only loads things close to the function it analyzes, instead of needing a global view.

Scales in what way? Memory usage? I just watched Gabor's talk where, if I understood correctly, he mentions that the combined ASTs for llvm took up 40Gb when exported to disk. I'm assuming that's not compressed or anything. There are plenty of workstations with 40Gb+ of RAM. The static analyzer may need significant additional memory for its processing, but my tool does not.

And tools like mine aren't part of the interactive development process. They may only ever need to be applied to a project once. So it might even be acceptable for it to take a week or whatever to execute. (I don't see why this wouldn't apply, to a lesser degree, to the static analyzer as well.)

Are you sure you're not being overly conservative about the scaling issue? Or am I somehow just being naive here? For me, I feel that it's a very inconvenient feature omission, and I'm not sure I really understand the reluctance.

Anyway, if I understood correctly, Gabor was implying that I could somehow use/abuse the new feature to achieve a combined AST? By importing all the functions in the other translation units manually? And if memory use really is an issue, is there a way to import a function into the AST, check it out, then lose it when your done with it?

Noah


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Cross Translation Unit Support in Clang

Sumner, Brian via cfe-dev
On Fri, Jun 30, 2017 at 4:03 PM Noah L <[hidden email]> wrote:

On Thu, Jun 29, 2017 at 10:31 PM, Manuel Klimek <[hidden email]> wrote:
...


I'm not saying it's not feasible. I'm not even saying it's not reasonable. But you can see why it'd be nicer for me if libTooling could just present me with a combined AST. :)

Unfortunately that only scales to small projects in the generalized case. The static analyzer gets away with it because it only loads things close to the function it analyzes, instead of needing a global view.

Scales in what way? Memory usage?

Both memory and compute.
 
I just watched Gabor's talk where, if I understood correctly, he mentions that the combined ASTs for llvm took up 40Gb when exported to disk. I'm assuming that's not compressed or anything. There are plenty of workstations with 40Gb+ of RAM.

There are also plenty of workstations that don't have that much RAM. I personally like tools that I can run on a machine without killing all other jobs, too :)
 
The static analyzer may need significant additional memory for its processing, but my tool does not.

And tools like mine aren't part of the interactive development process. They may only ever need to be applied to a project once. So it might even be acceptable for it to take a week or whatever to execute. (I don't see why this wouldn't apply, to a lesser degree, to the static analyzer as well.)
 
Generally, in my experience, for development speed, sanity and error resilience, faster turnaround times help.
 
Are you sure you're not being overly conservative about the scaling issue? Or am I somehow just being naive here? For me, I feel that it's a very inconvenient feature omission, and I'm not sure I really understand the reluctance.

We might disagree on the overhead of the 2-phase solution. We have built libtooling around this, and have quite a bit of infrastructure (that's also getting expanded over time) to make this as easy as possible.
 
Anyway, if I understood correctly, Gabor was implying that I could somehow use/abuse the new feature to achieve a combined AST? By importing all the functions in the other translation units manually? And if memory use really is an issue, is there a way to import a function into the AST, check it out, then lose it when your done with it?

Noah


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Cross Translation Unit Support in Clang

Sumner, Brian via cfe-dev


On 30 June 2017 at 16:21, Manuel Klimek <[hidden email]> wrote:
On Fri, Jun 30, 2017 at 4:03 PM Noah L <[hidden email]> wrote:

On Thu, Jun 29, 2017 at 10:31 PM, Manuel Klimek <[hidden email]> wrote:
...


I'm not saying it's not feasible. I'm not even saying it's not reasonable. But you can see why it'd be nicer for me if libTooling could just present me with a combined AST. :)

Unfortunately that only scales to small projects in the generalized case. The static analyzer gets away with it because it only loads things close to the function it analyzes, instead of needing a global view.

Scales in what way? Memory usage?

Both memory and compute.
 
I just watched Gabor's talk where, if I understood correctly, he mentions that the combined ASTs for llvm took up 40Gb when exported to disk. I'm assuming that's not compressed or anything. There are plenty of workstations with 40Gb+ of RAM.

There are also plenty of workstations that don't have that much RAM. I personally like tools that I can run on a machine without killing all other jobs, too :)

Also note that 40Gb on disk might not correspond to 40Gb+ RAM. One of the reason the dumps are so big because the headers are duplicated in the AST files of separate TUs. In case we utilize lazy loading from AST files (i.e. loading only the definitions we need), and merging the definitions into the original AST, we might save a lot of memory compared to the disk dumps. Or in case of modules, the duplication would be much less on the disks too.
 
 
The static analyzer may need significant additional memory for its processing, but my tool does not.

And tools like mine aren't part of the interactive development process. They may only ever need to be applied to a project once. So it might even be acceptable for it to take a week or whatever to execute. (I don't see why this wouldn't apply, to a lesser degree, to the static analyzer as well.)
 
Generally, in my experience, for development speed, sanity and error resilience, faster turnaround times help.
 
Are you sure you're not being overly conservative about the scaling issue? Or am I somehow just being naive here? For me, I feel that it's a very inconvenient feature omission, and I'm not sure I really understand the reluctance.

We might disagree on the overhead of the 2-phase solution. We have built libtooling around this, and have quite a bit of infrastructure (that's also getting expanded over time) to make this as easy as possible.
 
Anyway, if I understood correctly, Gabor was implying that I could somehow use/abuse the new feature to achieve a combined AST? By importing all the functions in the other translation units manually? And if memory use really is an issue, is there a way to import a function into the AST, check it out, then lose it when your done with it?

Noah



_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Cross Translation Unit Support in Clang

Sumner, Brian via cfe-dev
In reply to this post by Sumner, Brian via cfe-dev
On 26 June 2017 at 07:16, Manuel Klimek <[hidden email]> wrote:
On Mon, Jun 26, 2017 at 4:08 PM Gábor Horváth via cfe-dev <[hidden email]> wrote:
Hi,

Some benchmarks of the binary size after introducing the libTooling dependency to clang (using static linking on Linux):

Release:
85457072 -> 85505864
Debug:
1777932672 -> 1779938696

The increase is less than 1% in both cases. So, in my opinion, the binary size is not really a problem here.
Of course, the link times might increase a bit as well.

A question is, who should make the call whether this penalty is ok or not?

(after replying on the patch without noticing that this thread is not part of the patch, here it goes again :) 
Richard (cc'ed) usually owns decisions around clang itself. Writing an email to cfe-dev with the numbers and wait for whether others have concerns would probably also be good (many will probably not continue reading this thread).

I'm a bit lost as to what's actually being proposed here. It seems that there are at least three separate questions:

1) Does some kind of support for a database of multiple translation units belong in Clang? (The exact design of that database can be discussed separately.)
2) Should it be a separate library or part of libTooling?
3) Is it acceptable for the clang static analyser (and thus the clang binary) to link against that library?

My thoughts:

1: Yes, this seems like a sensible thing to have within the clang repository, particularly if there would be in-tree users of it. The clang static analyser would be one justification for this; another would be support of exported templates (if we ever want to be C++98 feature-complete). We'll need to discuss what the model is for this layer (multiple ASTContexts? one ASTContext modeling multiple TUs?), and how it fits in with other parts of Clang that solve similar problems (particularly ExternalASTSources, the serialization layer, ASTImporter).

2: I haven't seen anyone provide good justification for merging this library with libTooling, and the fact that the static analyser wants to use this and doesn't want tooling support strongly suggests to me that it should be separate.

3: The above binary size increases seem acceptable for a significant feature that the static analyser needs.
 
On 23 June 2017 at 22:06, Anna Zaks <[hidden email]> wrote:

On Jun 23, 2017, at 12:39 PM, Gábor Horváth <[hidden email]> wrote:

This is a dependency for the Static Analyzer component within clang (to another component, which is in clang as well).

OK. This means that we are talking about adding a dependency on libTooling to clang. This would not only effect the static analyzer, so we’d need an OK from a greater clang community.


There are 2 differences from the dependency on ASTMatchers:
 - Nothing that is presently in libTooling us used by clang.

This is actually not a difference. When the commit above was excepted, it was the first use of ASTMatchers in the clang binary.
 
 - The new component that the analyzer will depend on will be supporting a very experimental feature.

The other expedient options that I can see are:
 - Adding a separate library with just the functionality that this feature needs.
 - Making the dependency conditional, following the same style as Z3 support, and keeping it that way until the feature is fully implemented. This solution definitely has downsides.

Yet another solution is to pull the analyzer out of clang, but unfortunately that is non-trivial, so I am not sure if there are volunteers for the task.

I agree that pulling the analyzer out is a big task and would also break the backward compatibility of the command line. So it is not the best solution for the users.

Regards,
Gábor
 

Anna.

On 23 June 2017 at 19:48, Anna Zaks <[hidden email]> wrote:

On Jun 23, 2017, at 1:40 AM, Gábor Horváth <[hidden email]> wrote:

Anna,

Are you ok having libTooling as a dependency of the Static Analyzer?

Are we talking about introducing dependency for scan-build or clang itself?

I think this is not a bad direction since it has other good utilities that the Static Analyzer could use in the future such as Replacements, FixIts.

Regards,
Gábor

On 22 June 2017 at 12:10, Manuel Klimek <[hidden email]> wrote:
For clang tooling, I don't really expect us to do cross-TU AST loading outside of what modules will provide, as that inherently doesn't scale. Instead, we usually design pipelined approaches where the first "scan" over the codebase provides data which are reduced to the information needed for the tool.

That said, I can't predict the future, and people have expressed interest in that functionality, so
a) I agree it's a good idea to add and 
b) I think it's a good fit for libtooling

Cheers,
/Manuel


On Thu, Jun 22, 2017 at 11:58 AM Aleksei Sidorin <[hidden email]> wrote:
Hello Gabor,

Internally, we have created XTU module inside clang (lib/XTU). I think
it is the best way because importing is not related to analyzer
directly. We're not going to use it outside of CSA but I think future
users should have such possibility.

22.06.2017 12:41, Gábor Horváth пишет:
> Hi!
>
> It looks like there is a consensus to accept the cross translation
> unit analysis patch into the clang static analyzer:
> https://reviews.llvm.org/D30691
>
> There is one part of the patch that is independent of the static
> analyzer. The logic which can look up a function in an index and load
> a serialized AST that contains the definition of the function.
> The lookup is cached, and after the AST is loaded, the function
> definition will be merged into the original AST.
>
> Right now, in the current patch, this functionality is in the
> ASTContext. This is definitely not the right place to put this. So the
> question is, do you plan to utilize similar functionality in Clang
> tooling or clang tidy?
>
> If yes, we might end up creating a new module for that (or add it to
> an existing commonly used one like libtooling?). If no, we will move
> the corresponding code to the static analyzer.
>
> What do you think?
>
> In case you are interested in how it works, you can check out the
> EuroLLVM talk:
> http://llvm.org/devmtg/2017-03//2017/02/20/accepted-sessions.html#7
>
> Regards,
> Gábor


--
Best regards,
Aleksei Sidorin,
SRR, Samsung Electronics





_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Cross Translation Unit Support in Clang

Sumner, Brian via cfe-dev


On 5 July 2017 at 23:09, Richard Smith <[hidden email]> wrote:
On 26 June 2017 at 07:16, Manuel Klimek <[hidden email]> wrote:
On Mon, Jun 26, 2017 at 4:08 PM Gábor Horváth via cfe-dev <[hidden email]> wrote:
Hi,

Some benchmarks of the binary size after introducing the libTooling dependency to clang (using static linking on Linux):

Release:
85457072 -> 85505864
Debug:
1777932672 -> 1779938696

The increase is less than 1% in both cases. So, in my opinion, the binary size is not really a problem here.
Of course, the link times might increase a bit as well.

A question is, who should make the call whether this penalty is ok or not?

(after replying on the patch without noticing that this thread is not part of the patch, here it goes again :) 
Richard (cc'ed) usually owns decisions around clang itself. Writing an email to cfe-dev with the numbers and wait for whether others have concerns would probably also be good (many will probably not continue reading this thread).

I'm a bit lost as to what's actually being proposed here. It seems that there are at least three separate questions:

1) Does some kind of support for a database of multiple translation units belong in Clang? (The exact design of that database can be discussed separately.)
2) Should it be a separate library or part of libTooling?
3) Is it acceptable for the clang static analyser (and thus the clang binary) to link against that library?

My thoughts:

1: Yes, this seems like a sensible thing to have within the clang repository, particularly if there would be in-tree users of it. The clang static analyser would be one justification for this; another would be support of exported templates (if we ever want to be C++98 feature-complete). We'll need to discuss what the model is for this layer (multiple ASTContexts? one ASTContext modeling multiple TUs?), and how it fits in with other parts of Clang that solve similar problems (particularly ExternalASTSources, the serialization layer, ASTImporter).

This functionality basically using the serialization layer and ASTImporter, and provides the user with an easy interface to import function definitions from other TU and merge into the current one. This could be extended to any kinds of definitions once users need that.
 

2: I haven't seen anyone provide good justification for merging this library with libTooling, and the fact that the static analyser wants to use this and doesn't want tooling support strongly suggests to me that it should be separate.

Once we add on demand reparsing instead of loading AST files, the libTooling will be a dependency of this code. Moreover it does nothing static analyzer specific, this is the reason why we was thinking that libTooling is a good place for this functionality.

The currently proposed (libTooling) functionality can load certain interesting function definitions from external source into the current AST, so existing single TU tools can operate on the AST with more available information as if they had cross TU capabilities.
 

3: The above binary size increases seem acceptable for a significant feature that the static analyser needs.

Great news, thanks!

Regards,
Gábor
 
 
On 23 June 2017 at 22:06, Anna Zaks <[hidden email]> wrote:

On Jun 23, 2017, at 12:39 PM, Gábor Horváth <[hidden email]> wrote:

This is a dependency for the Static Analyzer component within clang (to another component, which is in clang as well).

OK. This means that we are talking about adding a dependency on libTooling to clang. This would not only effect the static analyzer, so we’d need an OK from a greater clang community.


There are 2 differences from the dependency on ASTMatchers:
 - Nothing that is presently in libTooling us used by clang.

This is actually not a difference. When the commit above was excepted, it was the first use of ASTMatchers in the clang binary.
 
 - The new component that the analyzer will depend on will be supporting a very experimental feature.

The other expedient options that I can see are:
 - Adding a separate library with just the functionality that this feature needs.
 - Making the dependency conditional, following the same style as Z3 support, and keeping it that way until the feature is fully implemented. This solution definitely has downsides.

Yet another solution is to pull the analyzer out of clang, but unfortunately that is non-trivial, so I am not sure if there are volunteers for the task.

I agree that pulling the analyzer out is a big task and would also break the backward compatibility of the command line. So it is not the best solution for the users.

Regards,
Gábor
 

Anna.

On 23 June 2017 at 19:48, Anna Zaks <[hidden email]> wrote:

On Jun 23, 2017, at 1:40 AM, Gábor Horváth <[hidden email]> wrote:

Anna,

Are you ok having libTooling as a dependency of the Static Analyzer?

Are we talking about introducing dependency for scan-build or clang itself?

I think this is not a bad direction since it has other good utilities that the Static Analyzer could use in the future such as Replacements, FixIts.

Regards,
Gábor

On 22 June 2017 at 12:10, Manuel Klimek <[hidden email]> wrote:
For clang tooling, I don't really expect us to do cross-TU AST loading outside of what modules will provide, as that inherently doesn't scale. Instead, we usually design pipelined approaches where the first "scan" over the codebase provides data which are reduced to the information needed for the tool.

That said, I can't predict the future, and people have expressed interest in that functionality, so
a) I agree it's a good idea to add and 
b) I think it's a good fit for libtooling

Cheers,
/Manuel


On Thu, Jun 22, 2017 at 11:58 AM Aleksei Sidorin <[hidden email]> wrote:
Hello Gabor,

Internally, we have created XTU module inside clang (lib/XTU). I think
it is the best way because importing is not related to analyzer
directly. We're not going to use it outside of CSA but I think future
users should have such possibility.

22.06.2017 12:41, Gábor Horváth пишет:
> Hi!
>
> It looks like there is a consensus to accept the cross translation
> unit analysis patch into the clang static analyzer:
> https://reviews.llvm.org/D30691
>
> There is one part of the patch that is independent of the static
> analyzer. The logic which can look up a function in an index and load
> a serialized AST that contains the definition of the function.
> The lookup is cached, and after the AST is loaded, the function
> definition will be merged into the original AST.
>
> Right now, in the current patch, this functionality is in the
> ASTContext. This is definitely not the right place to put this. So the
> question is, do you plan to utilize similar functionality in Clang
> tooling or clang tidy?
>
> If yes, we might end up creating a new module for that (or add it to
> an existing commonly used one like libtooling?). If no, we will move
> the corresponding code to the static analyzer.
>
> What do you think?
>
> In case you are interested in how it works, you can check out the
> EuroLLVM talk:
> http://llvm.org/devmtg/2017-03//2017/02/20/accepted-sessions.html#7
>
> Regards,
> Gábor


--
Best regards,
Aleksei Sidorin,
SRR, Samsung Electronics





_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev



_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Cross Translation Unit Support in Clang

Sumner, Brian via cfe-dev
On Thu, Jul 6, 2017 at 10:57 AM Gábor Horváth <[hidden email]> wrote:
On 5 July 2017 at 23:09, Richard Smith <[hidden email]> wrote:
On 26 June 2017 at 07:16, Manuel Klimek <[hidden email]> wrote:
On Mon, Jun 26, 2017 at 4:08 PM Gábor Horváth via cfe-dev <[hidden email]> wrote:
Hi,

Some benchmarks of the binary size after introducing the libTooling dependency to clang (using static linking on Linux):

Release:
85457072 -> 85505864
Debug:
1777932672 -> 1779938696

The increase is less than 1% in both cases. So, in my opinion, the binary size is not really a problem here.
Of course, the link times might increase a bit as well.

A question is, who should make the call whether this penalty is ok or not?

(after replying on the patch without noticing that this thread is not part of the patch, here it goes again :) 
Richard (cc'ed) usually owns decisions around clang itself. Writing an email to cfe-dev with the numbers and wait for whether others have concerns would probably also be good (many will probably not continue reading this thread).

I'm a bit lost as to what's actually being proposed here. It seems that there are at least three separate questions:

1) Does some kind of support for a database of multiple translation units belong in Clang? (The exact design of that database can be discussed separately.)
2) Should it be a separate library or part of libTooling?
3) Is it acceptable for the clang static analyser (and thus the clang binary) to link against that library?

My thoughts:

1: Yes, this seems like a sensible thing to have within the clang repository, particularly if there would be in-tree users of it. The clang static analyser would be one justification for this; another would be support of exported templates (if we ever want to be C++98 feature-complete). We'll need to discuss what the model is for this layer (multiple ASTContexts? one ASTContext modeling multiple TUs?), and how it fits in with other parts of Clang that solve similar problems (particularly ExternalASTSources, the serialization layer, ASTImporter).

This functionality basically using the serialization layer and ASTImporter, and provides the user with an easy interface to import function definitions from other TU and merge into the current one. This could be extended to any kinds of definitions once users need that.
 

2: I haven't seen anyone provide good justification for merging this library with libTooling, and the fact that the static analyser wants to use this and doesn't want tooling support strongly suggests to me that it should be separate.

Once we add on demand reparsing instead of loading AST files, the libTooling will be a dependency of this code. Moreover it does nothing static analyzer specific, this is the reason why we was thinking that libTooling is a good place for this functionality.

The currently proposed (libTooling) functionality can load certain interesting function definitions from external source into the current AST, so existing single TU tools can operate on the AST with more available information as if they had cross TU capabilities.

I think we can generally break everything that deals with running clang out of libtooling into an extra library.
 
3: The above binary size increases seem acceptable for a significant feature that the static analyser needs.

Great news, thanks!

Regards,
Gábor
 
 
On 23 June 2017 at 22:06, Anna Zaks <[hidden email]> wrote:

On Jun 23, 2017, at 12:39 PM, Gábor Horváth <[hidden email]> wrote:

This is a dependency for the Static Analyzer component within clang (to another component, which is in clang as well).

OK. This means that we are talking about adding a dependency on libTooling to clang. This would not only effect the static analyzer, so we’d need an OK from a greater clang community.


There are 2 differences from the dependency on ASTMatchers:
 - Nothing that is presently in libTooling us used by clang.

This is actually not a difference. When the commit above was excepted, it was the first use of ASTMatchers in the clang binary.
 
 - The new component that the analyzer will depend on will be supporting a very experimental feature.

The other expedient options that I can see are:
 - Adding a separate library with just the functionality that this feature needs.
 - Making the dependency conditional, following the same style as Z3 support, and keeping it that way until the feature is fully implemented. This solution definitely has downsides.

Yet another solution is to pull the analyzer out of clang, but unfortunately that is non-trivial, so I am not sure if there are volunteers for the task.

I agree that pulling the analyzer out is a big task and would also break the backward compatibility of the command line. So it is not the best solution for the users.

Regards,
Gábor
 

Anna.

On 23 June 2017 at 19:48, Anna Zaks <[hidden email]> wrote:

On Jun 23, 2017, at 1:40 AM, Gábor Horváth <[hidden email]> wrote:

Anna,

Are you ok having libTooling as a dependency of the Static Analyzer?

Are we talking about introducing dependency for scan-build or clang itself?

I think this is not a bad direction since it has other good utilities that the Static Analyzer could use in the future such as Replacements, FixIts.

Regards,
Gábor

On 22 June 2017 at 12:10, Manuel Klimek <[hidden email]> wrote:
For clang tooling, I don't really expect us to do cross-TU AST loading outside of what modules will provide, as that inherently doesn't scale. Instead, we usually design pipelined approaches where the first "scan" over the codebase provides data which are reduced to the information needed for the tool.

That said, I can't predict the future, and people have expressed interest in that functionality, so
a) I agree it's a good idea to add and 
b) I think it's a good fit for libtooling

Cheers,
/Manuel


On Thu, Jun 22, 2017 at 11:58 AM Aleksei Sidorin <[hidden email]> wrote:
Hello Gabor,

Internally, we have created XTU module inside clang (lib/XTU). I think
it is the best way because importing is not related to analyzer
directly. We're not going to use it outside of CSA but I think future
users should have such possibility.

22.06.2017 12:41, Gábor Horváth пишет:
> Hi!
>
> It looks like there is a consensus to accept the cross translation
> unit analysis patch into the clang static analyzer:
> https://reviews.llvm.org/D30691
>
> There is one part of the patch that is independent of the static
> analyzer. The logic which can look up a function in an index and load
> a serialized AST that contains the definition of the function.
> The lookup is cached, and after the AST is loaded, the function
> definition will be merged into the original AST.
>
> Right now, in the current patch, this functionality is in the
> ASTContext. This is definitely not the right place to put this. So the
> question is, do you plan to utilize similar functionality in Clang
> tooling or clang tidy?
>
> If yes, we might end up creating a new module for that (or add it to
> an existing commonly used one like libtooling?). If no, we will move
> the corresponding code to the static analyzer.
>
> What do you think?
>
> In case you are interested in how it works, you can check out the
> EuroLLVM talk:
> http://llvm.org/devmtg/2017-03//2017/02/20/accepted-sessions.html#7
>
> Regards,
> Gábor


--
Best regards,
Aleksei Sidorin,
SRR, Samsung Electronics





_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
12
Loading...