<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Mar 15, 2013 at 9:03 PM, Joshua Cranmer <span dir="ltr"><<a href="mailto:pidgeot18@gmail.com" target="_blank">pidgeot18@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"><div class="im">
<div>On 3/14/2013 3:42 PM, Sean Silva wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">On Thu, Mar 14, 2013 at 12:30 AM, Joshua Cranmer <span dir="ltr"><<a href="mailto:pidgeot18@gmail.com" target="_blank">pidgeot18@gmail.com</a>></span>
wrote:<br>
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div>On 3/13/2013 10:05 PM, Sean Silva wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">While
I'm all for the idea of improving the plugin API, I
think that a modest reduction in boilerplate is not
sufficiently compelling to foist a new plugin API on
people who already have existing code. The funny thing
about boilerplate is that it's easy to copy-paste, so
it doesn't really impede people from achieving their
goals since they can just copy the code that already
works. The primary problem of boilerplate is that it
has the effect of deterring newbies, and that issue
can be easily combated with improved documentation,
which avoids breaking every external plugin and
tutorial on plugins.<br>
</blockquote>
<br>
</div>
One thing that REALLY sucks with the current approach is
the need to specify clang -Xclang -load -Xclang <plugin
tarball> -Xclang -add-plugin <plugin name>
-Xclang -plugin-<name>-arg -Xclang <blah> ...<br>
With the new approach, the command line is clang
-fplugin=<tarball>
-fplugin-arg-<name>-<arg>=<blah>, which
is a much shorter command line and can actually be passed
into CFLAGS/CXXFLAGS without driving libtool bonkers (I
detest the need for wrapper scripts just to pass
arguments) and also eliminates warnings whenever people
use $(CXX) $(CXXFLAGS) as the linker.</blockquote>
<div><br>
</div>
<div>
<div><br>
Realistically a tiny script might be a better long-term
design, allowing e.g. "clang++ `clang-plugin-config
myPlugin.so arg1 arg2` foo.cpp". Remember that the
primary advantage of plugins vs libtooling/libclang is
that they are run as part of a build process, meaning
that in reality these command lines are meant to be
generated by a "configure" step and not by hand. So
really the "user friendliness" is determined by "how
easy is it to integrate a clang plugin into my build",
and not by the exact commandline syntax per se. This
kind of script could also serve as a useful layer of
indirection and "user friendliness", e.g. it could
recognize a "CLANG_PLUGIN_PATH" or other niceties that
would be dubious to add to clang itself.</div>
</div>
</div>
</div>
</div>
</blockquote>
<br></div>
A "tiny script" sounds to me a much worse option: you now have to
run two processes per compiler invocation instead of one (and on
Windows, process creation can take hundreds of milliseconds, so it's
a noticeable slowdown).</div></blockquote><div><br></div><div>The clang driver already forks at least one process per compiler invocation. Your comments apply equally to that and I don't see anybody running to fix them (or even complaining), so I'm not convinced that this is really as significant of an issue as you make it out to be. <br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000"> Also, you have to find more binaries to run
it: if I specify CXX via a path, how should a build system know
where to run clang-plugin-config from? You could guess by looking up
the dirname of CXX and hoping it's there, </div></blockquote><div><br></div><div>I'm not sure I follow your point here. I image clang-plugin-config and the wrapper to be installed next to clang and be looked up/executed as usual. </div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000">but you are also
advocating using shell scripts to represent CXX in another email,
which renders this approach impossible.</div></blockquote><div><br></div><div style>I also don't see the connection with my suggestion in the other email. In fact, the wrapper script for plugins and the compile_commands.json harvester could probably be the same script, and at configure time clang-plugin-config (or, perhaps better just `clang-config` now that it is going beyond plugins) would arrange for the wrapper script to perform the requested actions.</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000"><div class="im">
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">If you really want to immediately
push plugins forward in a big way, it would be monumental to
set up a buildbot that runs a clang plugin that does extra
checking that isn't really appropriate for being integrated
as a diagnostic into the compiler proper. For example, a
plugin that warns on incorrect uses of dyn_cast<>. For
maximum effect this should be developed in-tree (probably in
clang-tools-extra. Even though it has "tools" in the name, I
don't think anybody would be opposed to developing plugins
in there). It should also have an easy way for people in our
community to come up with and implement good extra checks
and get them integrated into that buildbot.</div>
</div>
</div>
</blockquote>
<br></div>
I am working on adding a compiler static checker plugin to Mozilla
that would check the guarantees our old dehydra plugin used to
check: a "must override" annotation (all subclasses must provide
their own implementation of this method), a "stack class" annotation
(this class cannot be allocated from the heap), and a warning that
gets emitted every time you emit a static initializer.<div class="im"><br></div></div></blockquote><div><br></div><div style>Awesome. Please keep us up to date with this work. Some of these checks seem like they could be relevant to llvm/clang too.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000"><div class="im">
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">The
changes in this patch retain almost all of the same
functionality as the original plugin approach (including
the ability to do things like add custom compile passes
via the RegisterPass statics) while wrapping it in a much
saner wrapper.
<div>
<br>
</div>
</blockquote>
<div><br>
</div>
<div>My opposition to the current patch is that it
does not provide enough value to our users to compensate
for the inconvenience that it will cause them (by breaking
their code). My opposition is not technical; I don't doubt
that your approach here is an improvement from a purely
technical standpoint.</div>
</div>
</div>
</div>
</blockquote>
<br></div>
The current plugin approach presumes that it is a pure consumer of
the AST, which isn't a viable option in my opinion. One thing I
would like to do in the future is be able to map Decls in the AST to
functions emitted in the LLVM IR, which is completely impossible
under the current architecture. Note also that I'm not removing the
current (more or less broken) plugin architecture, so I'm not
compelling people to switch.</div></blockquote><div><br></div><div style>You did delete the only code (PrintFunctionNames) in tree that AFAIK tests the previous functionality, which I interpreted as meaning that it was dead to you.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000">Rather, this is about enabling future
changes that permit plugins to not take the view that they happen
independently of code generation.</div></blockquote><div> </div><div style>This did not get through to me from the OP. Could you explain how the design you implement in this patch achieves that? It should be the emphasis of the review (and IMHO warrants a "does this direction and implementation approach sound good to everyone" cfe-dev discussion before proposing code to be committed).</div>
<div style><br></div><div style>Also, the command line parsing stuff should be in a separate patch, and IMO the -fplugin should be just a driver arg: that way, the previous commandline args for plugins (directly via cc1) remains in a live code path.</div>
<div style><br></div><div style>As I said earlier, the compatibilty stuff also deserves a rehash, since I'm still not convinced that it is really useful.</div><div style><br></div><div style>-- Sean Silva</div></div><br>
</div></div>