<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Aug 22, 2014 at 10:42 AM, Reid Kleckner <span dir="ltr"><<a href="mailto:rnk@google.com" target="_blank">rnk@google.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 dir="ltr">Looks good. Originally I made it virtual to try to handle the different ownership models of LLVM and Clangs argument parsing, but it's less trouble to just unify them.</div>
</blockquote><div><br></div><div>As you probably saw, this got reverted due to some more StringSaver's in LLD, one of which was problematic because it took a lock in SaveString (and Rui was on vacation). That use (COFFObjectReader::handleDirectiveSection) also had a weird ownership situation with the StringSaver design I implemented in this patch (and the existing one as it was being used); this issue is making me reconsider the design. Basically, it seems like it would be better to allocate the strings to the PECOFFLinkingContext's BumpPtrAllocator, rather than the allocator owned in the StringSaver itself.</div>
<div><br></div><div>One alternative is for StringSaver to hold a reference to a BumpPtrAllocator (instead of owning it) and just facilitate the Alloc.Allocate<char> + memcpy pattern. However, that makes me wonder if it would be better to just have a saveCStr(BumpPtrAllocator&, const char *) free function.</div>
<div><br></div><div>That approach however loses the nice semantic touch of having a function take a StringSaver& in interfaces (instead of a BumpPtrAllocator&) which clearly indicates that they are just saving strings.</div>
<div><br></div><div>Any thoughts? I can move forward with the design in this patch. It's just that this leaves a sort of suboptimal situation (no worse than it is currently though) in COFFObjectReader::handleDirectiveSection.</div>
<div><br></div><div>-- Sean Silva</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 class="gmail_extra">
<br><br>
<div class="gmail_quote"><div class="">On Fri, Aug 15, 2014 at 4:18 PM, Sean Silva <span dir="ltr"><<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>></span> wrote:<br></div><div><div class="h5">
<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">
Author: silvas<br>
Date: Fri Aug 15 18:18:33 2014<br>
New Revision: 215784<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=215784&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=215784&view=rev</a><br>
Log:<br>
[Support] Promote cl::StringSaver to a separate utility<br>
<br>
This class is generally useful.<br>
<br>
In breaking it out, the primary change is that it has been made<br>
non-virtual. It seems like being abstract led to there being 3 different<br>
(2 in llvm + 1 in clang) concrete implementations which disagreed about<br>
the ownership of the saved strings (see the manual call to free() in the<br>
unittest StrDupSaver; yes this is different from the CommandLine.cpp<br>
StrDupSaver which owns the stored strings; which is different from<br>
Clang's StringSetSaver which just holds a reference to a<br>
std::set<std::string> which owns the strings).<br>
<br>
I've identified 2 other places in the<br>
codebase that are open-coding this pattern:<br>
<br>
memcpy(Alloc.Allocate<char>(strlen(S)+1), S, strlen(S)+1)<br>
<br>
I'll be switching them over. They are<br>
* llvm::sys::Process::GetArgumentVector<br>
* The StringAllocator member of YAMLIO's Input class<br>
This also will allow simplifying Clang's driver.cpp quite a bit.<br>
<br>
Let me know if there are any other places that could benefit from<br>
StringSaver. I'm also thinking of adding a saveStringRef member for<br>
getting a stable StringRef.<br>
<br>
Added:<br>
llvm/trunk/include/llvm/Support/StringSaver.h<br>
Modified:<br>
llvm/trunk/include/llvm/Support/CommandLine.h<br>
llvm/trunk/lib/Support/CommandLine.cpp<br>
llvm/trunk/unittests/Support/CommandLineTest.cpp<br>
<br>
Modified: llvm/trunk/include/llvm/Support/CommandLine.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/CommandLine.h?rev=215784&r1=215783&r2=215784&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/CommandLine.h?rev=215784&r1=215783&r2=215784&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/include/llvm/Support/CommandLine.h (original)<br>
+++ llvm/trunk/include/llvm/Support/CommandLine.h Fri Aug 15 18:18:33 2014<br>
@@ -24,6 +24,7 @@<br>
#include "llvm/ADT/StringMap.h"<br>
#include "llvm/ADT/Twine.h"<br>
#include "llvm/Support/Compiler.h"<br>
+#include "llvm/Support/StringSaver.h"<br>
#include <cassert><br>
#include <climits><br>
#include <cstdarg><br>
@@ -1772,15 +1773,6 @@ void getRegisteredOptions(StringMap<Opti<br>
// Standalone command line processing utilities.<br>
//<br>
<br>
-/// \brief Saves strings in the inheritor's stable storage and returns a stable<br>
-/// raw character pointer.<br>
-class StringSaver {<br>
- virtual void anchor();<br>
-public:<br>
- virtual const char *SaveString(const char *Str) = 0;<br>
- virtual ~StringSaver() {}; // Pacify -Wnon-virtual-dtor.<br>
-};<br>
-<br>
/// \brief Tokenizes a command line that can contain escapes and quotes.<br>
//<br>
/// The quoting rules match those used by GCC and other tools that use<br>
<br>
Added: llvm/trunk/include/llvm/Support/StringSaver.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/StringSaver.h?rev=215784&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/StringSaver.h?rev=215784&view=auto</a><br>
==============================================================================<br>
--- llvm/trunk/include/llvm/Support/StringSaver.h (added)<br>
+++ llvm/trunk/include/llvm/Support/StringSaver.h Fri Aug 15 18:18:33 2014<br>
@@ -0,0 +1,33 @@<br>
+//===- llvm/Support/StringSaver.h - Stable storage for strings --*- C++ -*-===//<br>
+//<br>
+// The LLVM Compiler Infrastructure<br>
+//<br>
+// This file is distributed under the University of Illinois Open Source<br>
+// License. See LICENSE.TXT for details.<br>
+//<br>
+//===----------------------------------------------------------------------===//<br>
+<br>
+#ifndef LLVM_SUPPORT_STRINGSAVER_H<br>
+#define LLVM_SUPPORT_STRINGSAVER_H<br>
+<br>
+#include "llvm/Support/Allocator.h"<br>
+#include <cstring><br>
+<br>
+namespace llvm {<br>
+<br>
+/// \brief Saves strings in stable storage that it owns.<br>
+class StringSaver {<br>
+ BumpPtrAllocator Alloc;<br>
+<br>
+public:<br>
+ const char *saveCStr(const char *CStr) {<br>
+ auto Len = std::strlen(CStr) + 1; // Don't forget the NUL!<br>
+ char *Buf = Alloc.Allocate<char>(Len);<br>
+ std::memcpy(Buf, CStr, Len);<br>
+ return Buf;<br>
+ }<br>
+};<br>
+<br>
+} // end namespace llvm<br>
+<br>
+#endif<br>
<br>
Modified: llvm/trunk/lib/Support/CommandLine.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/CommandLine.cpp?rev=215784&r1=215783&r2=215784&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/CommandLine.cpp?rev=215784&r1=215783&r2=215784&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Support/CommandLine.cpp (original)<br>
+++ llvm/trunk/lib/Support/CommandLine.cpp Fri Aug 15 18:18:33 2014<br>
@@ -76,7 +76,6 @@ void parser<double>::anchor() {}<br>
void parser<float>::anchor() {}<br>
void parser<std::string>::anchor() {}<br>
void parser<char>::anchor() {}<br>
-void StringSaver::anchor() {}<br>
<br>
//===----------------------------------------------------------------------===//<br>
<br>
@@ -509,7 +508,7 @@ void cl::TokenizeGNUCommandLine(StringRe<br>
// End the token if this is whitespace.<br>
if (isWhitespace(Src[I])) {<br>
if (!Token.empty())<br>
- NewArgv.push_back(Saver.SaveString(Token.c_str()));<br>
+ NewArgv.push_back(Saver.saveCStr(Token.c_str()));<br>
Token.clear();<br>
continue;<br>
}<br>
@@ -520,7 +519,7 @@ void cl::TokenizeGNUCommandLine(StringRe<br>
<br>
// Append the last token after hitting EOF with no whitespace.<br>
if (!Token.empty())<br>
- NewArgv.push_back(Saver.SaveString(Token.c_str()));<br>
+ NewArgv.push_back(Saver.saveCStr(Token.c_str()));<br>
}<br>
<br>
/// Backslashes are interpreted in a rather complicated way in the Windows-style<br>
@@ -593,7 +592,7 @@ void cl::TokenizeWindowsCommandLine(Stri<br>
if (State == UNQUOTED) {<br>
// Whitespace means the end of the token.<br>
if (isWhitespace(Src[I])) {<br>
- NewArgv.push_back(Saver.SaveString(Token.c_str()));<br>
+ NewArgv.push_back(Saver.saveCStr(Token.c_str()));<br>
Token.clear();<br>
State = INIT;<br>
continue;<br>
@@ -625,7 +624,7 @@ void cl::TokenizeWindowsCommandLine(Stri<br>
}<br>
// Append the last token after hitting EOF with no whitespace.<br>
if (!Token.empty())<br>
- NewArgv.push_back(Saver.SaveString(Token.c_str()));<br>
+ NewArgv.push_back(Saver.saveCStr(Token.c_str()));<br>
}<br>
<br>
static bool ExpandResponseFile(const char *FName, StringSaver &Saver,<br>
@@ -691,25 +690,6 @@ bool cl::ExpandResponseFiles(StringSaver<br>
return AllExpanded;<br>
}<br>
<br>
-namespace {<br>
- class StrDupSaver : public StringSaver {<br>
- std::vector<char*> Dups;<br>
- public:<br>
- ~StrDupSaver() {<br>
- for (std::vector<char *>::iterator I = Dups.begin(), E = Dups.end();<br>
- I != E; ++I) {<br>
- char *Dup = *I;<br>
- free(Dup);<br>
- }<br>
- }<br>
- const char *SaveString(const char *Str) override {<br>
- char *Dup = strdup(Str);<br>
- Dups.push_back(Dup);<br>
- return Dup;<br>
- }<br>
- };<br>
-}<br>
-<br>
/// ParseEnvironmentOptions - An alternative entry point to the<br>
/// CommandLine library, which allows you to read the program's name<br>
/// from the caller (as PROGNAME) and its command-line arguments from<br>
@@ -729,8 +709,8 @@ void cl::ParseEnvironmentOptions(const c<br>
// Get program's "name", which we wouldn't know without the caller<br>
// telling us.<br>
SmallVector<const char *, 20> newArgv;<br>
- StrDupSaver Saver;<br>
- newArgv.push_back(Saver.SaveString(progName));<br>
+ StringSaver Saver;<br>
+ newArgv.push_back(Saver.saveCStr(progName));<br>
<br>
// Parse the value of the environment variable into a "command line"<br>
// and hand it off to ParseCommandLineOptions().<br>
@@ -754,7 +734,7 @@ void cl::ParseCommandLineOptions(int arg<br>
SmallVector<const char *, 20> newArgv;<br>
for (int i = 0; i != argc; ++i)<br>
newArgv.push_back(argv[i]);<br>
- StrDupSaver Saver;<br>
+ StringSaver Saver;<br>
ExpandResponseFiles(Saver, TokenizeGNUCommandLine, newArgv);<br>
argv = &newArgv[0];<br>
argc = static_cast<int>(newArgv.size());<br>
<br>
Modified: llvm/trunk/unittests/Support/CommandLineTest.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/CommandLineTest.cpp?rev=215784&r1=215783&r2=215784&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Support/CommandLineTest.cpp?rev=215784&r1=215783&r2=215784&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/unittests/Support/CommandLineTest.cpp (original)<br>
+++ llvm/trunk/unittests/Support/CommandLineTest.cpp Fri Aug 15 18:18:33 2014<br>
@@ -146,26 +146,19 @@ TEST(CommandLineTest, UseOptionCategory)<br>
"Category.";<br>
}<br>
<br>
-class StrDupSaver : public cl::StringSaver {<br>
- const char *SaveString(const char *Str) override {<br>
- return strdup(Str);<br>
- }<br>
-};<br>
-<br>
-typedef void ParserFunction(StringRef Source, llvm::cl::StringSaver &Saver,<br>
+typedef void ParserFunction(StringRef Source, StringSaver &Saver,<br>
SmallVectorImpl<const char *> &NewArgv);<br>
<br>
<br>
void testCommandLineTokenizer(ParserFunction *parse, const char *Input,<br>
const char *const Output[], size_t OutputSize) {<br>
SmallVector<const char *, 0> Actual;<br>
- StrDupSaver Saver;<br>
+ StringSaver Saver;<br>
parse(Input, Saver, Actual);<br>
EXPECT_EQ(OutputSize, Actual.size());<br>
for (unsigned I = 0, E = Actual.size(); I != E; ++I) {<br>
if (I < OutputSize)<br>
EXPECT_STREQ(Output[I], Actual[I]);<br>
- free(const_cast<char *>(Actual[I]));<br>
}<br>
}<br>
<br>
<br>
<br>
_______________________________________________<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>
</blockquote></div></div></div><br></div>
</blockquote></div><br></div></div>