Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
361733002:
Update Subzero to start parsing PNaCl bitcode files. (Closed)
|
Created:
6 years, 5 months ago by Karl Modified:
6 years, 5 months ago CC:
native-client-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/native_client/pnacl-subzero.git@master Visibility:
Public. |
DescriptionUpdate Subzero to start parsing PNaCl bitcode files.
This patch only handles global addresses in PNaCl bitcode files.
Function blocks are still not parsed. Also, factors out a common API
for translation, so that generated ICE can always be translated using
the same code.
BUG= https://code.google.com/p/nativeclient/issues/detail?id=3892
R=jvoung@chromium.org, stichnot@chromium.org
Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=8d7abae
Patch Set 1 #Patch Set 2 : Fix nits. #Patch Set 3 : Fix more nits. #
Total comments: 70
Patch Set 4 : Fix issues raised by Jan in patch set 1. #
Total comments: 16
Patch Set 5 : Fix issues raised in patch set 4. #Patch Set 6 : Fix some nits in patch set 5. #Patch Set 7 : Use SmallVector to hold data. #
Total comments: 48
Patch Set 8 : Fix nits in patch set 7. #Patch Set 9 : Reformat IceConverter.cpp #
Total comments: 1
Messages
Total messages: 12 (0 generated)
This CL is ready for review.
Jim - You should look at the following files, in that I
slightly restructured the code:
llvm2ice.cpp
IceTranslator.{h,cpp}
IceConverter.{h,cpp}
Jan - Could you please review the bitcode parsers I am
using to parse global addresses (from bitcode) in
PNaClTranslator.cpp
Thanks.
https://codereview.chromium.org/361733002/diff/40001/Makefile.standalone File Makefile.standalone (right): https://codereview.chromium.org/361733002/diff/40001/Makefile.standalone#newc... Makefile.standalone:33: $(OPTLEVEL) -g $(LLVM_CXXFLAGS) -m32 -Wno-error=unused-parameter Can you avoid filtering out the warning? Currently we are even adding warning checks w/ -Wextra. https://codereview.chromium.org/361733002/diff/40001/src/IceTranslator.h File src/IceTranslator.h (right): https://codereview.chromium.org/361733002/diff/40001/src/IceTranslator.h#newc... src/IceTranslator.h:10: // This file declares the general driver class for translating IR to ICE. Clarify what IR to ICE? I guess it has to be serialized bitcode IR, but both LLVM flavor and PNaCl flavor? Does it do more than just translate IR to ICE? Also translate ICE to machine code? https://codereview.chromium.org/361733002/diff/40001/src/IceTranslator.h#newc... src/IceTranslator.h:19: #include "IceGlobalContext.h" Could probably fwd declare the IceGlobalContext class. I think usually Jim just #include "IceDefs.h", which has a bunch of the fwd declarations... but that also means that if you modify IceDefs.h you have to recompile everything. https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp File src/PNaClTranslator.cpp (right): https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:37: void operator=(const TopLevelParser&) LLVM_DELETED_FUNCTION; I think for Subzero, Jim has been using: T &operator=(const T&) LLVM_DELETED_FUNCTION instead of void operator= though yes, about 2/3 of llvm uses void and the other 1/3 uses T & https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:40: TopLevelParser(Module *Mod, It would be good to clarify what state the incoming Module is in. It seems like it should be mostly empty, but it's hard to tell from just looking. See below -- should this parser just allocate the module on its own, and the fact that it even has to allocate a module is private implementation detail? https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:46: NumErrors(), initialize to 0? https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:69: size_t getHeaderSize() { Some of these other methods are const too? https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:85: Type *Ty = ID < TypeIDValues.size() ? TypeIDValues[ID] : 0; Clarify that the array could really end up storing 0 as some of its in-bound entries? Otherwise this check could have been simpler. https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:119: unsigned getNumFunctionIDs() { const https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:144: if (GlobalVarPlaceHolderType == 0) Why not just eagerly initialize it in the class's ctor? https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:206: raw_string_ostream StrBuf(Buffer); This probably doesn't count as "Subzero core" since it still uses a bunch of LLVM bits, but Jim was trying to avoid using raw_string_ostream in "Subzero core": https://codereview.chromium.org/353553004/diff/20001/src/IceTargetLoweringX86... Perhaps it was code-size concerns? Will have to ask to clarify. I do know that the plain C++ stringstream pulls in a hefty chunk of code (I think adding 500KB to 1MB to statically linked nexes). https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:239: std::string getRecordAddress() const { could inline this into Error(), then you don't need a separate buffer and raw_string_ostream https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:258: BlockParser(unsigned BlockID, TopLevelParser *Context) Why not make this first, before the nested block ctor, then it will be a contiguous section of protected: code, rather than have a small island of public: in the middle? https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:273: // The context parser that contains the decoded state. Can we collect the fields in one place, and the methods in another instead of interleaving them? https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:278: /// reported. Otherwise false. I feel like some of these comments about the error case can be shared between the next few methods. https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:339: bool BlockParser::ParseBlock(unsigned BlockID) { I wonder if this class should be called something that more clearly indicates that it's a base class and doesn't actually handle the details. Otherwise it seems unintuitive that a "BlockParser::ParseBlock" doesn't actually parse blocks =) https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:462: SmallVector<Constant *, 10> Initializers; Similar, can we put all the fields in one block, and methods in another? Helps eyeball which fields exist + their types, and therefore which fields need to be initialized by the ctor. https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:500: if (InitializersNeeded == Initializers.size()) { Would it be safer to check if >= ? Otherwise, it looks like it's just about right and odd that it would be an error. https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:503: StrBuf << RecordName << " record: Too many initializers, ignoring."; Probably could have just std::string() + ... for this. https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:550: if (NextGlobalID > Context->getNumFunctionIDs()) { I can't put my finger on it, but this checks seems unintuitive. NextGlobalID is initialized to getNumFunctionIDs, so perhaps the check should be for exact equality? https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:564: IsConstant = Values[0] != 0; Values[1] != 0 Otherwise, the alignment could make you think it's constant =) https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:601: for (unsigned i = 0; i < Size; ++i) could memcpy this? https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:644: BlockParser *EnclosingParser, indent to line up to after the ( ? There is "make -f Makefile.standalone format" to run clang-format over the code. Beware of it modifying unrelated files though. https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:672: // VST_ENTRY: [valid, namechar x N] valueID, or something instead of valid https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:674: ConvertToString(); Feels a bit roundabout that ConvertedName is a field within the ValuesymtabParser, and needs to be .clear()'ed each round. It's "parser state" but pretty limited in scope. It's really only live for one record, so really only live during ProcessRecord? https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:805: bool Results = Parser.ParseThisBlock(); Singular Result? Otherwise it sounds like a collection of results (e.g., at "return Results"). a "bool" can only count to 1 =) https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:809: errs() << "[" << i << "]: " << *ValueIDValues[i] << "\n"; How will this eventually transition? This starts to call translateFcn() directly, with some amount of construction of llvm::Types and some amount of conversion from llvm::Type to Ice::Type for globals and function signatures, and then function bodies are constructed as Ice::Cfg and Ice::Inst "directly" (but if the Ice::Inst refers to a global it'll also convert the LLVM type to Ice Type? https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:856: TopLevelParser Parser(&*Mod, Header, InputStream); Mod.get(), if you just want the raw pointer. Or maybe it should just pass the ownership over. Or maybe the Parser should be the thing constructing and owning the module the whole time, not get it passed in. Then, when we are able to avoid LLVM types there won't be a module to create at all. https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.h File src/PNaClTranslator.h (right): https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.h#ne... src/PNaClTranslator.h:1: //===- subzero/src/PNaClTranlator.h - Builds ICE from PNaCl bitcode -------===// PNaClTranslator.h https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.h#ne... src/PNaClTranslator.h:14: #ifndef SUBZERO_SRC_ICEREADER_H if-def guards don't match the filename I guess clang 3.4's -Wheader-guard won't catch that =) https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.h#ne... src/PNaClTranslator.h:26: // status 0 if successul. Nonzero otherwise. successful https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.h#ne... src/PNaClTranslator.h:27: int translate(std::string IRFilename); Could this be: "const std::string &" ?
https://codereview.chromium.org/361733002/diff/40001/Makefile.standalone File Makefile.standalone (right): https://codereview.chromium.org/361733002/diff/40001/Makefile.standalone#newc... Makefile.standalone:33: $(OPTLEVEL) -g $(LLVM_CXXFLAGS) -m32 -Wno-error=unused-parameter On 2014/07/01 17:32:52, jvoung wrote: > Can you avoid filtering out the warning? Currently we are even adding warning > checks w/ -Wextra. Added macro LLVM_UNUSED to individually turn this off for inline functions that don't use parameter arguments. Then removed flag. https://codereview.chromium.org/361733002/diff/40001/src/IceTranslator.h File src/IceTranslator.h (right): https://codereview.chromium.org/361733002/diff/40001/src/IceTranslator.h#newc... src/IceTranslator.h:10: // This file declares the general driver class for translating IR to ICE. On 2014/07/01 17:32:52, jvoung wrote: > Clarify what IR to ICE? I guess it has to be serialized bitcode IR, but both > LLVM flavor and PNaCl flavor? > > Does it do more than just translate IR to ICE? Also translate ICE to machine > code? Rewrote this to try and be more clear. https://codereview.chromium.org/361733002/diff/40001/src/IceTranslator.h#newc... src/IceTranslator.h:19: #include "IceGlobalContext.h" On 2014/07/01 17:32:52, jvoung wrote: > Could probably fwd declare the IceGlobalContext class. > > I think usually Jim just #include "IceDefs.h", which has a bunch of the fwd > declarations... but that also means that if you modify IceDefs.h you have to > recompile everything. Removing unnecessary includes. https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp File src/PNaClTranslator.cpp (right): https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:37: void operator=(const TopLevelParser&) LLVM_DELETED_FUNCTION; On 2014/07/01 17:32:52, jvoung wrote: > I think for Subzero, Jim has been using: > > T &operator=(const T&) LLVM_DELETED_FUNCTION > instead of void operator= > > though yes, about 2/3 of llvm uses void and the other 1/3 uses T & Done. https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:40: TopLevelParser(Module *Mod, On 2014/07/01 17:32:52, jvoung wrote: > It would be good to clarify what state the incoming Module is in. It seems like > it should be mostly empty, but it's hard to tell from just looking. > > See below -- should this parser just allocate the module on its own, and the > fact that it even has to allocate a module is private implementation detail? I hadn't really worried about this because these classes are in an unnamed namespace, and hence, are not visible outside this file. I thought the construction (down in PNaClTranslator::translate) was sufficient. However, I will move it. https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:46: NumErrors(), On 2014/07/01 17:32:53, jvoung wrote: > initialize to 0? Definitely! https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:69: size_t getHeaderSize() { On 2014/07/01 17:32:53, jvoung wrote: > Some of these other methods are const too? Done. https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:85: Type *Ty = ID < TypeIDValues.size() ? TypeIDValues[ID] : 0; On 2014/07/01 17:32:53, jvoung wrote: > Clarify that the array could really end up storing 0 as some of its in-bound > entries? Otherwise this check could have been simpler. Done. https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:119: unsigned getNumFunctionIDs() { On 2014/07/01 17:32:52, jvoung wrote: > const Done. https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:144: if (GlobalVarPlaceHolderType == 0) On 2014/07/01 17:32:53, jvoung wrote: > Why not just eagerly initialize it in the class's ctor? Done. https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:206: raw_string_ostream StrBuf(Buffer); On 2014/07/01 17:32:52, jvoung wrote: > This probably doesn't count as "Subzero core" since it still uses a bunch of > LLVM bits, but Jim was trying to avoid using raw_string_ostream in "Subzero > core": > https://codereview.chromium.org/353553004/diff/20001/src/IceTargetLoweringX86... > > Perhaps it was code-size concerns? Will have to ask to clarify. > > I do know that the plain C++ stringstream pulls in a hefty chunk of code (I > think adding 500KB to 1MB to statically linked nexes). First off, Jim's comment about not using it is incorrect. He uses it a lot in IceConverter.cpp. Hence, it will be pulled into llvm2ice. BTW, raw_string_ostream is really relatively small (if you look at the code), and is way smaller than the C++ stringstreams. Also, errs() and outs() define much smaller footprints than std::cout and std::cerr, but Jim is using those as well. https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:239: std::string getRecordAddress() const { On 2014/07/01 17:32:52, jvoung wrote: > could inline this into Error(), then you don't need a separate buffer and > raw_string_ostream Ok. the other uses I had for it has since been removed. https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:258: BlockParser(unsigned BlockID, TopLevelParser *Context) On 2014/07/01 17:32:52, jvoung wrote: > Why not make this first, before the nested block ctor, then it will be a > contiguous section of protected: code, rather than have a small island of > public: in the middle? Done. https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:273: // The context parser that contains the decoded state. On 2014/07/01 17:32:53, jvoung wrote: > Can we collect the fields in one place, and the methods in another instead of > interleaving them? Done. https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:278: /// reported. Otherwise false. On 2014/07/01 17:32:53, jvoung wrote: > I feel like some of these comments about the error case can be shared between > the next few methods. Done. https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:339: bool BlockParser::ParseBlock(unsigned BlockID) { On 2014/07/01 17:32:53, jvoung wrote: > I wonder if this class should be called something that more clearly indicates > that it's a base class and doesn't actually handle the details. > > Otherwise it seems unintuitive that a "BlockParser::ParseBlock" doesn't actually > parse blocks =) Done. https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:462: SmallVector<Constant *, 10> Initializers; On 2014/07/01 17:32:53, jvoung wrote: > Similar, can we put all the fields in one block, and methods in another? > > Helps eyeball which fields exist + their types, and therefore which fields need > to be initialized by the ctor. Done. https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:500: if (InitializersNeeded == Initializers.size()) { On 2014/07/01 17:32:52, jvoung wrote: > Would it be safer to check if >= ? Otherwise, it looks like it's just about > right and odd that it would be an error. I did this so that we wouldn't get cascading errors if multiple extra initializers were provided. However, cascading errors might be appropriate as well. https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:503: StrBuf << RecordName << " record: Too many initializers, ignoring."; On 2014/07/01 17:32:52, jvoung wrote: > Probably could have just std::string() + ... for this. Done. https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:550: if (NextGlobalID > Context->getNumFunctionIDs()) { On 2014/07/01 17:32:52, jvoung wrote: > I can't put my finger on it, but this checks seems unintuitive. > > NextGlobalID is initialized to getNumFunctionIDs, so perhaps the check should be > for exact equality? The test is to verify it appears before any globals are defined. Since it is an error test, I modified it to inequality. https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:564: IsConstant = Values[0] != 0; On 2014/07/01 17:32:52, jvoung wrote: > Values[1] != 0 > > Otherwise, the alignment could make you think it's constant =) Done. https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:601: for (unsigned i = 0; i < Size; ++i) On 2014/07/01 17:32:53, jvoung wrote: > could memcpy this? Not really. We are doing a cast from uint64_t to uint8_t on each element. However, its clear this isn't obvious. Adding cast to clarify. https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:644: BlockParser *EnclosingParser, On 2014/07/01 17:32:53, jvoung wrote: > indent to line up to after the ( ? > > There is "make -f Makefile.standalone format" to run clang-format over the code. > Beware of it modifying unrelated files though. Done. https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:672: // VST_ENTRY: [valid, namechar x N] On 2014/07/01 17:32:53, jvoung wrote: > valueID, or something instead of valid Done. https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:674: ConvertToString(); On 2014/07/01 17:32:52, jvoung wrote: > Feels a bit roundabout that ConvertedName is a field within the > ValuesymtabParser, and needs to be .clear()'ed each round. It's "parser state" > but pretty limited in scope. It's really only live for one record, so really > only live during ProcessRecord? I mainly did this to avoid allocations between calls. However, since we are using SmallString, this is unlikely. Fixing. https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:805: bool Results = Parser.ParseThisBlock(); On 2014/07/01 17:32:53, jvoung wrote: > Singular Result? Otherwise it sounds like a collection of results (e.g., at > "return Results"). > > a "bool" can only count to 1 =) > Done. https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:809: errs() << "[" << i << "]: " << *ValueIDValues[i] << "\n"; On 2014/07/01 17:32:53, jvoung wrote: > How will this eventually transition? This starts to call translateFcn() > directly, with some amount of construction of llvm::Types and some amount of > conversion from llvm::Type to Ice::Type for globals and function signatures, and > then function bodies are constructed as Ice::Cfg and Ice::Inst "directly" (but > if the Ice::Inst refers to a global it'll also convert the LLVM type to Ice > Type? I need to implement a FunctionParser. It will have a local field containing the Cfg being built. The method FunctionParser::ProcessRecord() will add instructions to the Cfg as it processes each record (rather than building LLVM IR first). On block exit (i.e. in FunctionParser::ExitBlock()) it will cal translateFcn(). For global addresses, we will look up the corresponding LLVM value (in ValueIDValues) and do the conversion in place. I still haven't thought of the exact transition of LLVM types to Ice types. If it is simple enough, I will probably have the TypesParser build the types as they are parsed in (and store them in a vector). Otherwise, I will do it in FunctionParser. Note: I may also lift convertType and convertIntegerType (from IceConverter.cpp) to the base class (or as separate functions) so that we only have one set of conversion routines. https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:856: TopLevelParser Parser(&*Mod, Header, InputStream); On 2014/07/01 17:32:52, jvoung wrote: > Mod.get(), if you just want the raw pointer. > > Or maybe it should just pass the ownership over. > > Or maybe the Parser should be the thing constructing and owning the module the > whole time, not get it passed in. Then, when we are able to avoid LLVM types > there won't be a module to create at all. Moved into Parser constructor. https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.h File src/PNaClTranslator.h (right): https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.h#ne... src/PNaClTranslator.h:1: //===- subzero/src/PNaClTranlator.h - Builds ICE from PNaCl bitcode -------===// On 2014/07/01 17:32:53, jvoung wrote: > PNaClTranslator.h Done. https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.h#ne... src/PNaClTranslator.h:14: #ifndef SUBZERO_SRC_ICEREADER_H On 2014/07/01 17:32:54, jvoung wrote: > if-def guards don't match the filename > > I guess clang 3.4's -Wheader-guard won't catch that =) Done. https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.h#ne... src/PNaClTranslator.h:26: // status 0 if successul. Nonzero otherwise. On 2014/07/01 17:32:53, jvoung wrote: > successful Done. https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.h#ne... src/PNaClTranslator.h:27: int translate(std::string IRFilename); On 2014/07/01 17:32:53, jvoung wrote: > Could this be: "const std::string &" ? > Done.
Parser mostly looks okay -- main question is about the ExitStatus vs return value. https://codereview.chromium.org/361733002/diff/40001/src/IceTranslator.h File src/IceTranslator.h (right): https://codereview.chromium.org/361733002/diff/40001/src/IceTranslator.h#newc... src/IceTranslator.h:10: // This file declares the general driver class for translating IR to ICE. On 2014/07/01 21:31:06, Karl wrote: > On 2014/07/01 17:32:52, jvoung wrote: > > Clarify what IR to ICE? I guess it has to be serialized bitcode IR, but both > > LLVM flavor and PNaCl flavor? > > > > Does it do more than just translate IR to ICE? Also translate ICE to machine > > code? > > Rewrote this to try and be more clear. For this one-line summary, it feels like this class should be described as an ICE -> Machine code translator. (Also the one-line summary at the top/first line of this file, and the .cpp file). After all, that's all it exposes as an API. Some derived class conjures a Cfg, then calls translateFcn(Cfg) or emitConstants(). Your later description makes sense though ("Base class for translating ICE to machine code"). https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp File src/PNaClTranslator.cpp (right): https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:46: NumErrors(), On 2014/07/01 21:31:06, Karl wrote: > On 2014/07/01 17:32:53, jvoung wrote: > > initialize to 0? > > Definitely! Okay, using () does initialize to zero, but it seems inconsistent that NumErrors uses () while the other fields use (0). https://codereview.chromium.org/361733002/diff/60001/src/IceTranslator.cpp File src/IceTranslator.cpp (right): https://codereview.chromium.org/361733002/diff/60001/src/IceTranslator.cpp#ne... src/IceTranslator.cpp:10: // This file defines the general driver class for translating IR to ICE. Similar comment -- this file doesn't seem to touch the earlier IR at all (just ICE). https://codereview.chromium.org/361733002/diff/60001/src/IceTranslator.h File src/IceTranslator.h (right): https://codereview.chromium.org/361733002/diff/60001/src/IceTranslator.h#newc... src/IceTranslator.h:26: // Derived classes convert other intermediate representations downto ICE, down to ICE https://codereview.chromium.org/361733002/diff/60001/src/PNaClTranslator.cpp File src/PNaClTranslator.cpp (right): https://codereview.chromium.org/361733002/diff/60001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:42: // Module *Mod, can remove commented out argument now https://codereview.chromium.org/361733002/diff/60001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:592: uint8_t *Buf = new uint8_t[Size]; Would it help w/ allocations a bit if this was a smallvector? https://codereview.chromium.org/361733002/diff/60001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:627: // If reached, just processed another intializer. See if time initializer https://codereview.chromium.org/361733002/diff/60001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:845: if (Parser.Parse()) return 1; ExitStatus not set for this early return https://codereview.chromium.org/361733002/diff/60001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:855: return ExitStatus = (Parser.getNumErrors() > 0); What if the parser has 0 errors, but translateFcn for some function in the middle of the steram has an error? (When this code path starts using translateFcn). Will that end up overwriting ExitStatus? Seems like we should clean this up: * There's a separate ExitStatus field, but the caller for this code path doesn't actually care to check it, because there's also a return value. * However, the return value may diverge from the ExitStatus field. - One way is to go all-in on the ExitStatus field and kill the return value (have the caller check ExitStatus instead)... - Another way is to go all-in on the return value. E.g., translateFcn() could forget about the ExitStatus field too, and return an 1/0. One thing to be careful of is that I think the idea is that translateFcn() might be allowed to continue processing more functions, even though there was an error earlier, just to see all the errors that might show up. - Other ideas?
https://codereview.chromium.org/361733002/diff/40001/Makefile.standalone File Makefile.standalone (right): https://codereview.chromium.org/361733002/diff/40001/Makefile.standalone#newc... Makefile.standalone:33: $(OPTLEVEL) -g $(LLVM_CXXFLAGS) -m32 -Wno-error=unused-parameter On 2014/07/01 21:31:06, Karl wrote: > On 2014/07/01 17:32:52, jvoung wrote: > > Can you avoid filtering out the warning? Currently we are even adding warning > > checks w/ -Wextra. > > Added macro LLVM_UNUSED to individually turn this off for inline functions that > don't use parameter arguments. Then removed flag. Adding flag back, since (at least at the moment) we need to change things upstream in LLVM in order to remove this flag. https://codereview.chromium.org/361733002/diff/40001/src/IceTranslator.h File src/IceTranslator.h (right): https://codereview.chromium.org/361733002/diff/40001/src/IceTranslator.h#newc... src/IceTranslator.h:10: // This file declares the general driver class for translating IR to ICE. On 2014/07/02 17:00:15, jvoung wrote: > On 2014/07/01 21:31:06, Karl wrote: > > On 2014/07/01 17:32:52, jvoung wrote: > > > Clarify what IR to ICE? I guess it has to be serialized bitcode IR, but both > > > LLVM flavor and PNaCl flavor? > > > > > > Does it do more than just translate IR to ICE? Also translate ICE to machine > > > code? > > > > Rewrote this to try and be more clear. > > > For this one-line summary, it feels like this class should be described as an > ICE -> Machine code translator. (Also the one-line summary at the top/first line > of this file, and the .cpp file). > > After all, that's all it exposes as an API. Some derived class conjures a Cfg, > then calls translateFcn(Cfg) or emitConstants(). > > Your later description makes sense though ("Base class for translating ICE to > machine code"). Good point. Fixing. https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp File src/PNaClTranslator.cpp (right): https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:46: NumErrors(), On 2014/07/02 17:00:15, jvoung wrote: > On 2014/07/01 21:31:06, Karl wrote: > > On 2014/07/01 17:32:53, jvoung wrote: > > > initialize to 0? > > > > Definitely! > > Okay, using () does initialize to zero, but it seems inconsistent that NumErrors > uses () while the other fields use (0). Adding zero to be more clear. https://codereview.chromium.org/361733002/diff/60001/src/IceTranslator.cpp File src/IceTranslator.cpp (right): https://codereview.chromium.org/361733002/diff/60001/src/IceTranslator.cpp#ne... src/IceTranslator.cpp:10: // This file defines the general driver class for translating IR to ICE. On 2014/07/02 17:00:15, jvoung wrote: > Similar comment -- this file doesn't seem to touch the earlier IR at all (just > ICE). Done. https://codereview.chromium.org/361733002/diff/60001/src/IceTranslator.h File src/IceTranslator.h (right): https://codereview.chromium.org/361733002/diff/60001/src/IceTranslator.h#newc... src/IceTranslator.h:26: // Derived classes convert other intermediate representations downto ICE, On 2014/07/02 17:00:15, jvoung wrote: > down to ICE Done. https://codereview.chromium.org/361733002/diff/60001/src/PNaClTranslator.cpp File src/PNaClTranslator.cpp (right): https://codereview.chromium.org/361733002/diff/60001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:42: // Module *Mod, On 2014/07/02 17:00:16, jvoung wrote: > can remove commented out argument now Done. https://codereview.chromium.org/361733002/diff/60001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:592: uint8_t *Buf = new uint8_t[Size]; On 2014/07/02 17:00:16, jvoung wrote: > Would it help w/ allocations a bit if this was a smallvector? Actually, I forgot that in C++ you can create a local whose size is defined at runtime. Using uint8_t Buf[Size]; https://codereview.chromium.org/361733002/diff/60001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:627: // If reached, just processed another intializer. See if time On 2014/07/02 17:00:16, jvoung wrote: > initializer Done. https://codereview.chromium.org/361733002/diff/60001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:845: if (Parser.Parse()) return 1; On 2014/07/02 17:00:15, jvoung wrote: > ExitStatus not set for this early return Good point. I didn't realize that I had that field. Fixing so that ExitStatus always gets set, and then return. https://codereview.chromium.org/361733002/diff/60001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:855: return ExitStatus = (Parser.getNumErrors() > 0); On 2014/07/02 17:00:15, jvoung wrote: > What if the parser has 0 errors, but translateFcn for some function in the > middle of the steram has an error? (When this code path starts using > translateFcn). > > Will that end up overwriting ExitStatus? > > Seems like we should clean this up: > * There's a separate ExitStatus field, but the caller for this code path doesn't > actually care to check it, because there's also a return value. > * However, the return value may diverge from the ExitStatus field. > > - One way is to go all-in on the ExitStatus field and kill the return value > (have the caller check ExitStatus instead)... > - Another way is to go all-in on the return value. E.g., translateFcn() could > forget about the ExitStatus field too, and return an 1/0. One thing to be > careful of is that I think the idea is that translateFcn() might be allowed to > continue processing more functions, even though there was an error earlier, just > to see all the errors that might show up. > - Other ideas? Fixing so that when an error is reported (during parsing), ExitStatus is updated. Also, as you suggested, instead of using return value, change so that ExitStatus is used by caller (in llvm2ice).
https://codereview.chromium.org/361733002/diff/60001/src/PNaClTranslator.cpp File src/PNaClTranslator.cpp (right): https://codereview.chromium.org/361733002/diff/60001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:592: uint8_t *Buf = new uint8_t[Size]; On 2014/07/02 18:09:54, Karl wrote: > On 2014/07/02 17:00:16, jvoung wrote: > > Would it help w/ allocations a bit if this was a smallvector? > > Actually, I forgot that in C++ you can create a local whose size is defined at > runtime. Using > > uint8_t Buf[Size]; Actually, let's avoid variable length arrays. We had trouble building w/ those on Mac: https://codereview.chromium.org/335343005/
https://codereview.chromium.org/361733002/diff/60001/src/PNaClTranslator.cpp File src/PNaClTranslator.cpp (right): https://codereview.chromium.org/361733002/diff/60001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:592: uint8_t *Buf = new uint8_t[Size]; On 2014/07/02 18:47:53, jvoung wrote: > On 2014/07/02 18:09:54, Karl wrote: > > On 2014/07/02 17:00:16, jvoung wrote: > > > Would it help w/ allocations a bit if this was a smallvector? > > > > Actually, I forgot that in C++ you can create a local whose size is defined at > > runtime. Using > > > > uint8_t Buf[Size]; > > Actually, let's avoid variable length arrays. We had trouble building w/ those > on Mac: > > https://codereview.chromium.org/335343005/ Done.
otherwise the parser part LGTM https://codereview.chromium.org/361733002/diff/110001/src/IceTranslator.cpp File src/IceTranslator.cpp (right): https://codereview.chromium.org/361733002/diff/110001/src/IceTranslator.cpp#n... src/IceTranslator.cpp:1: //===- subzero/src/IceTranslator.cpp - Translate IR to ICE ------*- C++ -*-===// ICE to machine code https://codereview.chromium.org/361733002/diff/110001/src/IceTranslator.h File src/IceTranslator.h (right): https://codereview.chromium.org/361733002/diff/110001/src/IceTranslator.h#new... src/IceTranslator.h:1: //===- subzero/src/IceTranslator.h - Translate IR to ICE --------*- C++ -*-===// Ice to machine code https://codereview.chromium.org/361733002/diff/110001/src/PNaClTranslator.cpp File src/PNaClTranslator.cpp (right): https://codereview.chromium.org/361733002/diff/110001/src/PNaClTranslator.cpp... src/PNaClTranslator.cpp:812: } // end of anonymous namespace https://codereview.chromium.org/361733002/diff/110001/src/PNaClTranslator.cpp... src/PNaClTranslator.cpp:867: } // end of namespace Ice
LGTM with some nits. https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp File src/PNaClTranslator.cpp (right): https://codereview.chromium.org/361733002/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:644: BlockParser *EnclosingParser, On 2014/07/01 17:32:53, jvoung wrote: > There is "make -f Makefile.standalone format" to run clang-format over the code. > Beware of it modifying unrelated files though. This (modifying unrelated files) is usually because different versions of clang-format may make slightly different formatting decisions. It would be nice if there were a practical way to force a specific clang-format version. https://codereview.chromium.org/361733002/diff/110001/src/IceClFlags.h File src/IceClFlags.h (right): https://codereview.chromium.org/361733002/diff/110001/src/IceClFlags.h#newcode1 src/IceClFlags.h:1: //===- subzero/src/IceClFlags.h - Cl Flags controlling translation --------===// Add the "*- C++ -*" stuff so that emacs understands this as a C++ file. Maybe need to shorten the description to something like "Cl flags for translation". https://codereview.chromium.org/361733002/diff/110001/src/IceConverter.cpp File src/IceConverter.cpp (right): https://codereview.chromium.org/361733002/diff/110001/src/IceConverter.cpp#ne... src/IceConverter.cpp:618: } Add "end of anonymous namespace" comment. https://codereview.chromium.org/361733002/diff/110001/src/IceConverter.h File src/IceConverter.h (right): https://codereview.chromium.org/361733002/diff/110001/src/IceConverter.h#newc... src/IceConverter.h:25: class Converter : public Ice::Translator { Is Ice:: necessary when it's already inside namespace Ice? (here and below) https://codereview.chromium.org/361733002/diff/110001/src/IceConverter.h#newc... src/IceConverter.h:31: }; Add private methods: Converter(const Converter &) LLVM_DELETED_FUNCTION; Converter &operator=(const Converter &) LLVM_DELETED_FUNCTION; https://codereview.chromium.org/361733002/diff/110001/src/IceTranslator.h File src/IceTranslator.h (right): https://codereview.chromium.org/361733002/diff/110001/src/IceTranslator.h#new... src/IceTranslator.h:43: int ExitStatus; Originally, ExitStatus was an int local variable declared inside main() just because main() returns int. Now that ExitStatus lives relatively far outside main(), I think it would be better to make it a bool and then let main() translation between bool and int. https://codereview.chromium.org/361733002/diff/110001/src/IceTranslator.h#new... src/IceTranslator.h:52: llvm::OwningPtr<Ice::Cfg> Func; Use Cfg instead of Ice::Cfg, here and below. https://codereview.chromium.org/361733002/diff/110001/src/IceTranslator.h#new... src/IceTranslator.h:60: }; Add private methods: Translator(const Translator &) LLVM_DELETED_FUNCTION; Translator &operator=(const Translator &) LLVM_DELETED_FUNCTION; https://codereview.chromium.org/361733002/diff/110001/src/PNaClTranslator.cpp File src/PNaClTranslator.cpp (right): https://codereview.chromium.org/361733002/diff/110001/src/PNaClTranslator.cpp... src/PNaClTranslator.cpp:48: Header(Header), It looks like you didn't run clang-format on this file? If not, it would be good to do it up front to avoid distracting formatting changes in the future. https://codereview.chromium.org/361733002/diff/110001/src/PNaClTranslator.cpp... src/PNaClTranslator.cpp:52: GlobalVarPlaceHolderType(0) { Use NULL instead of 0 for clarity. (Or just initialize it to Type::getInt8Ty(getLLVMContext()) if practical.) For justification, LLVM coding style says nothing about NULL versus 0, but Chromium points to Google which says to use NULL. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#0_and_nullptr/... https://codereview.chromium.org/361733002/diff/110001/src/PNaClTranslator.cpp... src/PNaClTranslator.cpp:95: Type *Ty = ID < TypeIDValues.size() ? TypeIDValues[ID] : 0; NULL instead of 0 for clarity https://codereview.chromium.org/361733002/diff/110001/src/PNaClTranslator.cpp... src/PNaClTranslator.cpp:102: if (ID < TypeIDValues.size() && TypeIDValues[ID] == 0) { NULL https://codereview.chromium.org/361733002/diff/110001/src/PNaClTranslator.cpp... src/PNaClTranslator.cpp:139: /// Resizes the list of of value IDs to include Count global of of --> of https://codereview.chromium.org/361733002/diff/110001/src/PNaClTranslator.cpp... src/PNaClTranslator.cpp:332: if(ExpectedSize > 1) StrBuf << "s"; if( --> if ( https://codereview.chromium.org/361733002/diff/110001/src/PNaClTranslator.cpp... src/PNaClTranslator.cpp:374: Type *Ty = 0; NULL https://codereview.chromium.org/361733002/diff/110001/src/PNaClTranslator.cpp... src/PNaClTranslator.cpp:424: if (Ty == 0) NULL https://codereview.chromium.org/361733002/diff/110001/src/PNaClTranslator.cpp... src/PNaClTranslator.cpp:510: Constant *Init = 0; NULL https://codereview.chromium.org/361733002/diff/110001/src/PNaClTranslator.h File src/PNaClTranslator.h (right): https://codereview.chromium.org/361733002/diff/110001/src/PNaClTranslator.h#n... src/PNaClTranslator.h:1: //===- subzero/src/PNaClTranslator.h - Builds ICE from PNaCl bitcode ------===// Add the C++ emacs mode indicator. https://codereview.chromium.org/361733002/diff/110001/src/PNaClTranslator.h#n... src/PNaClTranslator.h:25: PNaClTranslator(Ice::GlobalContext *Ctx, Ice::ClFlags &Flags) Remove Ice:: https://codereview.chromium.org/361733002/diff/110001/src/PNaClTranslator.h#n... src/PNaClTranslator.h:31: }; Disable default copy ctor and assignment operator. https://codereview.chromium.org/361733002/diff/110001/src/PNaClTranslator.h#n... src/PNaClTranslator.h:35: #endif // SUBZERO_SRC_PNACLTRANSLATOR_H Single space before comment. :)
Message was sent while issue was closed.
Committed patchset #9 manually as r8d7abae (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/361733002/diff/110001/src/IceClFlags.h File src/IceClFlags.h (right): https://codereview.chromium.org/361733002/diff/110001/src/IceClFlags.h#newcode1 src/IceClFlags.h:1: //===- subzero/src/IceClFlags.h - Cl Flags controlling translation --------===// On 2014/07/07 20:50:23, stichnot wrote: > Add the "*- C++ -*" stuff so that emacs understands this as a C++ file. Maybe > need to shorten the description to something like "Cl flags for translation". Done. https://codereview.chromium.org/361733002/diff/110001/src/IceConverter.cpp File src/IceConverter.cpp (right): https://codereview.chromium.org/361733002/diff/110001/src/IceConverter.cpp#ne... src/IceConverter.cpp:618: } On 2014/07/07 20:50:23, stichnot wrote: > Add "end of anonymous namespace" comment. Done. https://codereview.chromium.org/361733002/diff/110001/src/IceConverter.h File src/IceConverter.h (right): https://codereview.chromium.org/361733002/diff/110001/src/IceConverter.h#newc... src/IceConverter.h:25: class Converter : public Ice::Translator { On 2014/07/07 20:50:23, stichnot wrote: > Is Ice:: necessary when it's already inside namespace Ice? (here and below) Done. https://codereview.chromium.org/361733002/diff/110001/src/IceConverter.h#newc... src/IceConverter.h:31: }; On 2014/07/07 20:50:23, stichnot wrote: > Add private methods: > > Converter(const Converter &) LLVM_DELETED_FUNCTION; > Converter &operator=(const Converter &) LLVM_DELETED_FUNCTION; Done. https://codereview.chromium.org/361733002/diff/110001/src/IceTranslator.cpp File src/IceTranslator.cpp (right): https://codereview.chromium.org/361733002/diff/110001/src/IceTranslator.cpp#n... src/IceTranslator.cpp:1: //===- subzero/src/IceTranslator.cpp - Translate IR to ICE ------*- C++ -*-===// On 2014/07/02 22:07:33, jvoung wrote: > ICE to machine code Done. https://codereview.chromium.org/361733002/diff/110001/src/IceTranslator.h File src/IceTranslator.h (right): https://codereview.chromium.org/361733002/diff/110001/src/IceTranslator.h#new... src/IceTranslator.h:1: //===- subzero/src/IceTranslator.h - Translate IR to ICE --------*- C++ -*-===// On 2014/07/02 22:07:33, jvoung wrote: > Ice to machine code Done. https://codereview.chromium.org/361733002/diff/110001/src/IceTranslator.h#new... src/IceTranslator.h:43: int ExitStatus; On 2014/07/07 20:50:23, stichnot wrote: > Originally, ExitStatus was an int local variable declared inside main() just > because main() returns int. Now that ExitStatus lives relatively far outside > main(), I think it would be better to make it a bool and then let main() > translation between bool and int. Done. https://codereview.chromium.org/361733002/diff/110001/src/IceTranslator.h#new... src/IceTranslator.h:52: llvm::OwningPtr<Ice::Cfg> Func; On 2014/07/07 20:50:23, stichnot wrote: > Use Cfg instead of Ice::Cfg, here and below. Done. https://codereview.chromium.org/361733002/diff/110001/src/IceTranslator.h#new... src/IceTranslator.h:60: }; On 2014/07/07 20:50:23, stichnot wrote: > Add private methods: > > Translator(const Translator &) LLVM_DELETED_FUNCTION; > Translator &operator=(const Translator &) LLVM_DELETED_FUNCTION; Done. https://codereview.chromium.org/361733002/diff/110001/src/PNaClTranslator.cpp File src/PNaClTranslator.cpp (right): https://codereview.chromium.org/361733002/diff/110001/src/PNaClTranslator.cpp... src/PNaClTranslator.cpp:48: Header(Header), On 2014/07/07 20:50:24, stichnot wrote: > It looks like you didn't run clang-format on this file? If not, it would be > good to do it up front to avoid distracting formatting changes in the future. Letting formatter fix this. https://codereview.chromium.org/361733002/diff/110001/src/PNaClTranslator.cpp... src/PNaClTranslator.cpp:52: GlobalVarPlaceHolderType(0) { On 2014/07/07 20:50:24, stichnot wrote: > Use NULL instead of 0 for clarity. (Or just initialize it to > Type::getInt8Ty(getLLVMContext()) if practical.) > > For justification, LLVM coding style says nothing about NULL versus 0, but > Chromium points to Google which says to use NULL. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#0_and_nullptr/... Done. https://codereview.chromium.org/361733002/diff/110001/src/PNaClTranslator.cpp... src/PNaClTranslator.cpp:95: Type *Ty = ID < TypeIDValues.size() ? TypeIDValues[ID] : 0; On 2014/07/07 20:50:24, stichnot wrote: > NULL instead of 0 for clarity Done. https://codereview.chromium.org/361733002/diff/110001/src/PNaClTranslator.cpp... src/PNaClTranslator.cpp:102: if (ID < TypeIDValues.size() && TypeIDValues[ID] == 0) { On 2014/07/07 20:50:24, stichnot wrote: > NULL Done. https://codereview.chromium.org/361733002/diff/110001/src/PNaClTranslator.cpp... src/PNaClTranslator.cpp:139: /// Resizes the list of of value IDs to include Count global On 2014/07/07 20:50:24, stichnot wrote: > of of --> of Done. https://codereview.chromium.org/361733002/diff/110001/src/PNaClTranslator.cpp... src/PNaClTranslator.cpp:332: if(ExpectedSize > 1) StrBuf << "s"; On 2014/07/07 20:50:24, stichnot wrote: > if( --> if ( Done. https://codereview.chromium.org/361733002/diff/110001/src/PNaClTranslator.cpp... src/PNaClTranslator.cpp:374: Type *Ty = 0; On 2014/07/07 20:50:23, stichnot wrote: > NULL Done. https://codereview.chromium.org/361733002/diff/110001/src/PNaClTranslator.cpp... src/PNaClTranslator.cpp:424: if (Ty == 0) On 2014/07/07 20:50:24, stichnot wrote: > NULL Done. https://codereview.chromium.org/361733002/diff/110001/src/PNaClTranslator.cpp... src/PNaClTranslator.cpp:510: Constant *Init = 0; On 2014/07/07 20:50:24, stichnot wrote: > NULL Done. https://codereview.chromium.org/361733002/diff/110001/src/PNaClTranslator.cpp... src/PNaClTranslator.cpp:812: } On 2014/07/02 22:07:33, jvoung wrote: > // end of anonymous namespace Done. https://codereview.chromium.org/361733002/diff/110001/src/PNaClTranslator.cpp... src/PNaClTranslator.cpp:867: } On 2014/07/02 22:07:33, jvoung wrote: > // end of namespace Ice Done. https://codereview.chromium.org/361733002/diff/110001/src/PNaClTranslator.h File src/PNaClTranslator.h (right): https://codereview.chromium.org/361733002/diff/110001/src/PNaClTranslator.h#n... src/PNaClTranslator.h:1: //===- subzero/src/PNaClTranslator.h - Builds ICE from PNaCl bitcode ------===// On 2014/07/07 20:50:25, stichnot wrote: > Add the C++ emacs mode indicator. Done. https://codereview.chromium.org/361733002/diff/110001/src/PNaClTranslator.h#n... src/PNaClTranslator.h:25: PNaClTranslator(Ice::GlobalContext *Ctx, Ice::ClFlags &Flags) On 2014/07/07 20:50:24, stichnot wrote: > Remove Ice:: Done. https://codereview.chromium.org/361733002/diff/110001/src/PNaClTranslator.h#n... src/PNaClTranslator.h:31: }; On 2014/07/07 20:50:24, stichnot wrote: > Disable default copy ctor and assignment operator. Done. https://codereview.chromium.org/361733002/diff/110001/src/PNaClTranslator.h#n... src/PNaClTranslator.h:35: #endif // SUBZERO_SRC_PNACLTRANSLATOR_H On 2014/07/07 20:50:24, stichnot wrote: > Single space before comment. :) Letting the formatter decide.
Message was sent while issue was closed.
https://codereview.chromium.org/361733002/diff/150001/src/PNaClTranslator.cpp File src/PNaClTranslator.cpp (right): https://codereview.chromium.org/361733002/diff/150001/src/PNaClTranslator.cpp... src/PNaClTranslator.cpp:840: ExitStatus); I get a compiler error on this line. Apparent cause: src/PNaClTranslator.cpp:42:3: note: no known conversion for argument 4 from ‘bool’ to ‘int&’ |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
