<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">Coding Standards: <a href="http://llvm.org/docs/CodingStandards.html">http://llvm.org/docs/CodingStandards.html</a><div><br></div><div><div style="margin: 0.8em 0px 0.5em; font-family: 'Lucida Grande', 'Lucida Sans Unicode', Geneva, Verdana, sans-serif; font-size: 14px; line-height: 21px; text-align: left; background-color: rgb(255, 255, 255); position: static; z-index: auto; ">"The problem with anonymous namespaces is that they naturally want to encourage indentation of their body, and they reduce locality of reference: if you see a random function definition in a C++ file, it is easy to see if it is marked static, but seeing if it is in an anonymous namespace requires scanning a big chunk of the file."</div></div><div><br></div><div>Thus the function should be static.</div><div><br></div><div>"<span style="background-color: rgb(255, 255, 255); font-family: 'Lucida Grande', 'Lucida Sans Unicode', Geneva, Verdana, sans-serif; font-size: 14px; line-height: 21px; text-align: left; ">Static constructors and destructors (e.g. global variables whose types have a constructor or destructor) should not be added to the code base, and should be removed wherever possible. Besides</span><span style="background-color: rgb(255, 255, 255); font-family: 'Lucida Grande', 'Lucida Sans Unicode', Geneva, Verdana, sans-serif; font-size: 14px; line-height: 21px; text-align: left; "> </span><a class="reference external" href="http://yosefk.com/c++fqa/ctors.html#fqa-10.12" style="font-family: 'Lucida Grande', 'Lucida Sans Unicode', Geneva, Verdana, sans-serif; font-size: 14px; line-height: 21px; text-align: left; color: rgb(202, 121, 0); ">well known problems</a><span style="background-color: rgb(255, 255, 255); font-family: 'Lucida Grande', 'Lucida Sans Unicode', Geneva, Verdana, sans-serif; font-size: 14px; line-height: 21px; text-align: left; "> </span><span style="background-color: rgb(255, 255, 255); font-family: 'Lucida Grande', 'Lucida Sans Unicode', Geneva, Verdana, sans-serif; font-size: 14px; line-height: 21px; text-align: left; ">where the order of initialization is undefined between globals in different source files, the entire concept of static constructors is at odds with the common use case of LLVM as a library linked into a larger application."</span></div><div><br></div><div>We don't want static constructors unless we must.</div><div><br></div><div>"<span style="background-color: rgb(255, 255, 255); font-family: 'Lucida Grande', 'Lucida Sans Unicode', Geneva, Verdana, sans-serif; font-size: 14px; line-height: 21px; text-align: left; ">The use of</span><span style="background-color: rgb(255, 255, 255); font-family: 'Lucida Grande', 'Lucida Sans Unicode', Geneva, Verdana, sans-serif; font-size: 14px; line-height: 21px; text-align: left; "> </span><tt class="docutils literal" style="line-height: 21px; text-align: left; font-family: Consolas, 'Deja Vu Sans Mono', 'Bitstream Vera Sans Mono', monospace; font-size: 0.95em; "><span class="pre">#include</span> <span class="pre"><iostream></span></tt><span style="background-color: rgb(255, 255, 255); font-family: 'Lucida Grande', 'Lucida Sans Unicode', Geneva, Verdana, sans-serif; font-size: 14px; line-height: 21px; text-align: left; "> </span><span style="background-color: rgb(255, 255, 255); font-family: 'Lucida Grande', 'Lucida Sans Unicode', Geneva, Verdana, sans-serif; font-size: 14px; line-height: 21px; text-align: left; ">in library files is hereby</span><span style="background-color: rgb(255, 255, 255); font-family: 'Lucida Grande', 'Lucida Sans Unicode', Geneva, Verdana, sans-serif; font-size: 14px; line-height: 21px; text-align: left; "> </span><strong style="font-family: 'Lucida Grande', 'Lucida Sans Unicode', Geneva, Verdana, sans-serif; font-size: 14px; line-height: 21px; text-align: left; ">forbidden</strong><span style="background-color: rgb(255, 255, 255); font-family: 'Lucida Grande', 'Lucida Sans Unicode', Geneva, Verdana, sans-serif; font-size: 14px; line-height: 21px; text-align: left; ">, because many common implementations transparently inject a</span><span style="background-color: rgb(255, 255, 255); font-family: 'Lucida Grande', 'Lucida Sans Unicode', Geneva, Verdana, sans-serif; font-size: 14px; line-height: 21px; text-align: left; "> </span><a class="reference internal" href="http://llvm.org/docs/CodingStandards.html#static-constructor" style="font-family: 'Lucida Grande', 'Lucida Sans Unicode', Geneva, Verdana, sans-serif; font-size: 14px; line-height: 21px; text-align: left; color: rgb(202, 121, 0); ">static constructor</a><span style="background-color: rgb(255, 255, 255); font-family: 'Lucida Grande', 'Lucida Sans Unicode', Geneva, Verdana, sans-serif; font-size: 14px; line-height: 21px; text-align: left; "> </span><span style="background-color: rgb(255, 255, 255); font-family: 'Lucida Grande', 'Lucida Sans Unicode', Geneva, Verdana, sans-serif; font-size: 14px; line-height: 21px; text-align: left; ">into every translation unit that includes it."</span></div><div><span style="background-color: rgb(255, 255, 255); font-family: 'Lucida Grande', 'Lucida Sans Unicode', Geneva, Verdana, sans-serif; font-size: 14px; line-height: 21px; text-align: left; ">"</span><span style="background-color: rgb(255, 255, 255); font-family: 'Lucida Grande', 'Lucida Sans Unicode', Geneva, Verdana, sans-serif; font-size: 14px; line-height: 21px; text-align: left; ">LLVM includes a lightweight, simple, and efficient stream implementation in</span><span style="background-color: rgb(255, 255, 255); font-family: 'Lucida Grande', 'Lucida Sans Unicode', Geneva, Verdana, sans-serif; font-size: 14px; line-height: 21px; text-align: left; "> </span><tt class="docutils literal" style="line-height: 21px; text-align: left; font-family: Consolas, 'Deja Vu Sans Mono', 'Bitstream Vera Sans Mono', monospace; font-size: 0.95em; ">llvm/Support/raw_ostream.h</tt><span style="background-color: rgb(255, 255, 255); font-family: 'Lucida Grande', 'Lucida Sans Unicode', Geneva, Verdana, sans-serif; font-size: 14px; line-height: 21px; text-align: left; ">, which provides all of the common features of</span><span style="background-color: rgb(255, 255, 255); font-family: 'Lucida Grande', 'Lucida Sans Unicode', Geneva, Verdana, sans-serif; font-size: 14px; line-height: 21px; text-align: left; "> </span><tt class="docutils literal" style="line-height: 21px; text-align: left; font-family: Consolas, 'Deja Vu Sans Mono', 'Bitstream Vera Sans Mono', monospace; font-size: 0.95em; ">std::ostream</tt><span style="background-color: rgb(255, 255, 255); font-family: 'Lucida Grande', 'Lucida Sans Unicode', Geneva, Verdana, sans-serif; font-size: 14px; line-height: 21px; text-align: left; ">. All new code should use</span><span style="background-color: rgb(255, 255, 255); font-family: 'Lucida Grande', 'Lucida Sans Unicode', Geneva, Verdana, sans-serif; font-size: 14px; line-height: 21px; text-align: left; "> </span><tt class="docutils literal" style="line-height: 21px; text-align: left; font-family: Consolas, 'Deja Vu Sans Mono', 'Bitstream Vera Sans Mono', monospace; font-size: 0.95em; ">raw_ostream</tt><span style="background-color: rgb(255, 255, 255); font-family: 'Lucida Grande', 'Lucida Sans Unicode', Geneva, Verdana, sans-serif; font-size: 14px; line-height: 21px; text-align: left; "> </span><span style="background-color: rgb(255, 255, 255); font-family: 'Lucida Grande', 'Lucida Sans Unicode', Geneva, Verdana, sans-serif; font-size: 14px; line-height: 21px; text-align: left; ">instead of</span><span style="background-color: rgb(255, 255, 255); font-family: 'Lucida Grande', 'Lucida Sans Unicode', Geneva, Verdana, sans-serif; font-size: 14px; line-height: 21px; text-align: left; "> </span><tt class="docutils literal" style="line-height: 21px; text-align: left; font-family: Consolas, 'Deja Vu Sans Mono', 'Bitstream Vera Sans Mono', monospace; font-size: 0.95em; ">ostream</tt><span style="background-color: rgb(255, 255, 255); font-family: 'Lucida Grande', 'Lucida Sans Unicode', Geneva, Verdana, sans-serif; font-size: 14px; line-height: 21px; text-align: left; ">."</span></div><div><br></div><div>raw_ostream seems to be an example of avoiding the static constructor problem (and others). In raw_ostream.cpp:</div><div> /// errs() - This returns a reference to a raw_ostream for standard error.</div><div> /// Use it like: errs() << "foo" << "bar";</div><div> raw_ostream &llvm::errs() {</div><div> // Set standard error to be unbuffered by default.</div><div> static raw_fd_ostream S(STDERR_FILENO, false, true);</div><div> return S;</div><div> }</div><div><br></div><div>Thus I believe using a static local is the preferred pattern to use over static constructor, when you must have that behavior. Only users pay the cost of initialization.</div><div><br></div><div>Of course, if your data structure is not truly global/static (i.e. it should be tied to a context), then using either a static global or static local is wrong, and it needs to be put in a corresponding context. I don't know your intent here, but it does seems to be truly global.</div><div><br></div><div><br><div><div>On Oct 1, 2013, at 11:23 AM, Puyan Lotfi <<a href="mailto:plotfi@apple.com">plotfi@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><meta http-equiv="Content-Type" content="text/html charset=windows-1252"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">Oh man. I actually wanted to do that too, but I thought it would be frowned upon stylistically.<div>You think that’d be ok?</div><div><br></div><div>-Puyan<br><div><br><div><div>On Oct 1, 2013, at 11:19 AM, Michael Ilseman <<a href="mailto:milseman@apple.com">milseman@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><meta http-equiv="Content-Type" content="text/html charset=windows-1252"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">Ah, my mistake. The LLVM Coding Standards uses my exact mistake as motivation for using of "static" instead of namespace on functions.<div><br></div><div>For the variable, doesn't this make it a static constructor (which is also recommended against)? Would it make sense to have it be a static local variable, so that the constructor only need run when the data structure is actually first used?<div><br><div><div>On Oct 1, 2013, at 11:14 AM, Puyan Lotfi <<a href="mailto:plotfi@apple.com">plotfi@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><meta http-equiv="Content-Type" content="text/html charset=windows-1252"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">I did, but it’s already inside the unnamed namespace so I didn’t; thought it does the same thing. <div>Do you think I should make PersistentClArgString and getPersistentClArgString() static for the sake of clarity?<div><br></div><div>-Puyan</div><div><br><div><div>On Oct 1, 2013, at 10:54 AM, Michael Ilseman <<a href="mailto:milseman@apple.com">milseman@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><meta http-equiv="Content-Type" content="text/html charset=windows-1252"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">Did you mean to make those static, or can they be referred to externally?<div><br><div><div>On Sep 30, 2013, at 8:10 PM, Puyan Lotfi <<a href="mailto:plotfi@apple.com">plotfi@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><meta http-equiv="Content-Type" content="text/html charset=windows-1252"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div>Reid,</div><div><br></div><div>After going over the code (specifically in libclang), I just can’t guarantee that all entry points into CompilerInvocation::CreateFromArgs() are passing arguments that aren’t stack or dynamically allocated then freed (For example, whatever calling clang::createInvocationFromCommandLine could pass in args pointing to strings from its stack). I think a practical solution is to have a persisting unique string pool in this instance. I’ve decided to change my code to use StringMap instead, so that should ease your concern over the complexity. If there is an easy to reuse global string pool within clang, I’d certainly like to know (I’ve heard that there are such pools in several LLVM projects for exactly this purpose, but I am not familiar with them). The global in this case is local to the file; I think for the sake of practicality it might be ok to allow this? Either that, or assume that CompilerInvocation instances are live for the lifetime of whatever main() is calling libclang because I’m not sure if StringRef on CodeGenOptions is any better of an assumption. </div><div><br></div><div>I am open to suggestions.</div><div><br></div><div>Thanks</div><div><br></div><div>Puyan</div><div><br></div><div></div></div><span><clangWorkaroundParseCommandLineOptionsStringMap.diff></span><meta http-equiv="Content-Type" content="text/html charset=windows-1252"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div></div><br><div><div>On Sep 30, 2013, at 2:47 PM, Reid Kleckner <<a href="mailto:rnk@google.com">rnk@google.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div dir="ltr">Can you try changing the std::strings in CodeGenOptions to StringRefs? The string persistance patch is n^2 and uses globals.</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Sep 30, 2013 at 2:24 PM, Puyan Lotfi <span dir="ltr"><<a href="mailto:plotfi@apple.com" target="_blank">plotfi@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div>Hi</div><div><br></div><div>As requested, I have ran the address sanitizer tests. I ran them many times, and could not find any reproducible failures.</div>
<div>I did notice some intermittent failures that came and went depending on what revision I had gotten from SVN.</div><div><br></div><div>Due to concern for use-after-free I have included a patch to clang that allows strings passed to ParseCommandLineOptions() from the CreateTargetMachine() function to persist (this was the only location in LLVM or clang where I found ParseCommandLineOptions being called with automatic memory); what it does is strdups the args for -debug-pass, -limit-float-precision as well as backend args and stores the result to a global vector of StringRefs. This vector is only populated for unique strings to avoid leaking repeatedly.</div>
<div><br></div><div>The other three patches include changes to the CommandLine library to enable StringRef (which I already sent out), changes to places using cl::opt<std::string> to use StringRef (also already sent out), and also the requested changes to the CommandLine documentation.</div>
<div><br></div><div>I think this should cover all the basis. I’ll check in if this looks ok to you all.</div><div><br></div><div>Thanks</div><span class="HOEnZb"><font color="#888888"><div><br></div><div>-Puyan</div><div>
<br></div><div><br></div><div></div></font></span></div><br><div style="word-wrap:break-word"><div></div></div>
<br><div style="word-wrap:break-word"><div></div></div>
<br><div style="word-wrap:break-word"><div></div></div>
<br><div style="word-wrap:break-word"><div></div><div><br></div><br><div><div>On Sep 23, 2013, at 10:43 AM, Reid Kleckner <<a href="mailto:rnk@google.com" target="_blank">rnk@google.com</a>> wrote:</div><br><blockquote type="cite">
<div dir="ltr">I worry that this patch creates a significant risk for use-after-free bugs. Can you:<div><br><div>1. Explain how string use-after-free is supposed to be avoided?</div><div>2. Update the docs if ParseCommandLine options is no longer going to save copies of strings.</div>
<div>3. Make sure the internal tests pass with an AddressSanitizer build, because I see things like ParseEnvironmentOptions which look broken with this change.</div></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">
On Mon, Sep 23, 2013 at 10:00 AM, Chris Lattner <span dir="ltr"><<a href="mailto:clattner@apple.com" target="_blank">clattner@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><br>
On Sep 19, 2013, at 3:01 PM, Puyan Lotfi <<a href="mailto:plotfi@apple.com" target="_blank">plotfi@apple.com</a>> wrote:<br>
<br>
> Hi All<br>
><br>
> I’ve improved this StringRef patch and I’d like some more feedback and hopefully a submission.<br>
> My changes to the original patch thus far have been:<br>
><br>
> - I added changes to CreateTargerMachine() in clang to have the SmallVector passed in from the caller.<br>
> - I found more places where cl::opt<str::string> was used, and I changed them to use cl::opt<StringRef>.<br>
><br>
> I’ve attached a patch that should apply cleanly with an llvm checkout that includes clang checked out into the tools directory.<br>
<br>
</div>This looks great to me. When you commit it, please split it into two pieces though:<br>
<br>
1) The part that adds cl compatibility with StringRef.<br>
2) The part that switches lots of cl::opt<> to use StringRef instead of std::string.<br>
<br>
Thanks Puyan,<br>
<br>
-Chris<br>
<div>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</div></blockquote></div><br></div>
</blockquote></div><br></div><br></blockquote></div><br></div>
</blockquote></div><br></div>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br></blockquote></div><br></div></div></blockquote></div><br></div></div></div></blockquote></div><br></div></div></div></blockquote></div><br></div></div></div></blockquote></div><br></div></body></html>