<div dir="ltr"><br><div class="gmail_extra"><div class="gmail_quote">On Sat, Mar 16, 2013 at 12:32 AM, 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/15/2013 11:01 PM, Sean Silva
wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">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>
</blockquote>
<br></div>
If I were to add a command in my Makefiles to spawn a $(shell), the
reviewers would throw a hissy fit. I'll also point out that Clang is
not a widely-used compilers on Windows systems, where this kind of
stuff matters more.<div class="im"><br></div></div></blockquote><div><br></div><div style>I'm not sure how I got derailed, but for the plugins there is no "wrapper script", just `clang-plugin-config myPlugin.so arg1 arg2` once at configure time. (the wrapper script would be for harvesting the compile commands, but that's a separate discussion and I'm not sure how it got brought into this one).</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">
<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>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>
</div>
</div>
</div>
</blockquote>
<br></div>
The logic I currently use to look up llvm-config for building the
plugin is as follows:<br>
if test -z "$CXX"; then<br>
CXX=`which clang++`<br>
fi<br>
if test -z "$LLVMCONFIG"; then<br>
LLVMCONFIG=`which llvm-config`<br>
fi<br>
if test -z "$LLVMCONFIG"; then<br>
LLVMCONFIG=`dirname $CXX`/llvm-config<br>
fi<br>
<br>
The ideas is to try to make this "just work" if the compiler to be
used is clang. However, if CXX is a shell script and clang is not
specifically in PATH (the latter case is not an esoteric
situation--it's how our own builders get to clang), then the value
returned is wrong. It's also wrong if people start using clang with
versioning numbers: consider clang symlinked to a clang-3.2, but
you're building with clang-3.3. Looking up llvm-config in the path
would find the llvm-config for 3.2 here instead of 3.3, which would
be wrong. IMHO, gcc's -print-file-name=plugin is much better (you
don't need to guess at the locations of other tools!).</div></blockquote><div><br></div><div style>Sorry I was confusing myself earlier. As I said before the "clang-plugin-config" runs once at configure time. Let's keep the discussion of the pros/cons of the wrapper script in the other thread.</div>
<div style><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">
<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">
<div bgcolor="#FFFFFF" text="#000000">
<div>
<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><br>
</div>
</div>
</blockquote>
<div><br>
</div>
<div>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>
</div>
</blockquote>
<br></div>
The biggest stumbling block to implementing useful checkers is the
inability to add custom annotations... annotate(string) is currently
being used as a hack, but what is really needed is the ability to
specify custom C++11 attributes. Actually committing the static
checker can be found in
<a href="https://bugzilla.mozilla.org/show_bug.cgi?id=767563" target="_blank"><https://bugzilla.mozilla.org/show_bug.cgi?id=767563></a>, but
there is a long list of desired analyses here
<a href="https://bugzilla.mozilla.org/buglist.cgi?list_id=6034150&resolution=---&query_format=advanced&component=Rewriting%20and%20Analysis&product=Core" target="_blank"><https://bugzilla.mozilla.org/buglist.cgi?list_id=6034150&resolution=---&query_format=advanced&component=Rewriting%20and%20Analysis&product=Core></a>.<div class="im">
<br>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<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>
<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>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>
</div>
</blockquote>
<br></div>
The old API I consider deprecated, but deprecated does not mean
imminent removal. Also, the examples directory isn't built by
default, so I doubt it's actually really being tested.<div class="im"><br>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<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>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>
</div>
</div>
</blockquote>
<br></div>
If you pay careful attention, you'd notice that the plugins are kept
around in the CompilerInstance object, which is passed around to all
the AST actions, including the CodeGen AST action; the old plugin
API stores everything as separate AST actions and instead
multiplexes all the AST actions together, so the CodeGen AST action
is unaware of the existence of plugins, short of creating Yet
Another Static Initializer attachment point.<div class="im"><br></div></div></blockquote><div><br></div><div style>Is there any way we could fuse the old functionality into the new functionality?</div><div style> </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">
<br>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div>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>
</div>
</div>
</blockquote>
<br></div>
That isn't feasible here, as the two plugin loading paths are
actually doing rather different things, and I don't think it is
desirable to attempt to merge what are conceptually different models
of plugins.<div class="im"><br>
<br>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div>As I said earlier, the compatibilty stuff also
deserves a rehash, since I'm still not convinced that it
is really useful.</div>
</div>
</div>
</div>
</blockquote></div>
The primary purpose of compatibility checking is to detect a
situation that would almost certainly lead to crash instead of
crashing. Users deserve to get useful error messages instead of
panicking crash dumps.<div class="im"><span style="color:rgb(34,34,34)"></span></div></div></blockquote><div><br></div><div style>It does nothing against subtle memory corruption, which is the real issue. A crash is a *best-case scenario* and is not a priority to protect against IMO. Blatant crashes are easy to catch and remedy. Regardless, it is deceptive to have something like DECLARE_PLUGIN_COMPATIBILITY() which still permits silent corruption to happen, even though the user thinks they are "protected".</div>
<div style><br></div><div style>The only satisfactory solution that I can think of is to have a special configure option that generates and embeds a unique ID (sha1sum of all the headers?) for a specific build which guarantees compatibility (it could be called ENABLE_PLUGINS, with the option ENABLE_UNSAFE_PLUGINS available to imitate the current plugin situation). ENABLE_PLUGINS not be on by default (presumably it would have some impact on build time), but we could document that when preparing binary packages of clang that clang should be built with this flag.</div>
<div style><br></div><div style><br></div><div style>-- Sean Silva</div></div></div></div>