Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
883673005:
Fix PNaCl bitcode reader to release global variables to emitter. (Closed)
|
Created:
5 years, 10 months ago by Karl Modified:
5 years, 10 months ago CC:
native-client-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/native_client/pnacl-subzero.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionFix PNaCl bitcode reader to release global variables to emitter.
Fixes the PNaCl bitcode reader to maintain two lists of global
variables. The first, VariableDeclarations, is the list of
variable declarations to be lowered by the emitter. The second,
ValueIDConstants, is the corresponding constant symbol to use
when references to the corresponding global variable declaration
is referenced when processing functions.
BUG=None
R=jvoung@chromium.org, stichnot@chromium.org
Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=6ca7d2b6bfa9e4bca707240208bba6612d58719d
Patch Set 1 #Patch Set 2 : Merge master. #Patch Set 3 : Fix nits. #
Total comments: 33
Patch Set 4 : Fix issues in patch set 3. #
Total comments: 19
Messages
Total messages: 9 (1 generated)
kschimpf@google.com changed reviewers: + jvoung@chromium.org, stichnot@chromium.org
Thanks! Also, please "make format". https://codereview.chromium.org/883673005/diff/40001/src/PNaClTranslator.cpp File src/PNaClTranslator.cpp (right): https://codereview.chromium.org/883673005/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:265: assert(VariableDeclarations.get()); You should be able to just assert(VariableDeclarations) because of operator bool(). It's good to minimize the uses of unique_ptr<>::get() so it's easier to reason about whether there are any stray copies of the pointer. https://codereview.chromium.org/883673005/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:271: assert(VariableDeclarations.get()); assert(VariableDeclarations); https://codereview.chromium.org/883673005/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:303: return FunctionDeclarationList.size() + Decls->size(); if (VariableDeclarations) { return FunctionDeclarationList.size() + VariableDeclarations->size(); https://codereview.chromium.org/883673005/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:311: assert(VariableDeclarations.get()); assert(VariableDeclarations); https://codereview.chromium.org/883673005/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:322: return Decls->size(); if (VariableDeclarations) { return VariableDeclarations->size(); https://codereview.chromium.org/883673005/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:330: assert(VariableDeclarations.get()); assert(VariableDeclarations); https://codereview.chromium.org/883673005/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:348: Ice::VariableDeclarationList *getGlobalVariables() { This would be better returning a unique_ptr. The last statement would be: return std::move(VariableDeclarations); https://codereview.chromium.org/883673005/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:352: assert(Decls->size() <= ValueIDConstants.size()); if (VariableDeclarations) { assert(VariableDeclarations->size() <= ...); or: assert(!VariableDeclarations || VariableDeclarations->size() <= ...); https://codereview.chromium.org/883673005/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:412: void installDeclarationName(// Ice::Translator &Trans, Remove comment in arglist? Same for the two calls below to this function? https://codereview.chromium.org/883673005/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:414: const Ice::IceString &Prefix, const char *Context, Bikeshedding. Maybe rename the Context arg, since there are already a couple of overloaded uses of this term? https://codereview.chromium.org/883673005/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:426: assert(VariableDeclarations.get()); assert(VariableDeclarations); https://codereview.chromium.org/883673005/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:430: // Ice::Translator &Trans = getTranslator(); remove? https://codereview.chromium.org/883673005/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:443: // Ice::Translator &Trans = getTranslator(); remove? https://codereview.chromium.org/883673005/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:468: for (size_t i = 0; i < NumFunctions; ++i) { Can you use a range-based for loop? https://codereview.chromium.org/883673005/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:482: for (size_t i = 0; i < NumGlobalVars; ++i) { Range-based for loop? https://codereview.chromium.org/883673005/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:2872: Context->getGlobalVariables()); With the suggested change to the getGlobalVariable() return type, this would be something like: std::unique_ptr<Ice::VariableDeclarationList> DeclsPtr = Context->getGlobalVariables();
https://codereview.chromium.org/883673005/diff/40001/src/PNaClTranslator.cpp File src/PNaClTranslator.cpp (right): https://codereview.chromium.org/883673005/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:265: assert(VariableDeclarations.get()); On 2015/02/10 04:01:04, stichnot wrote: > You should be able to just assert(VariableDeclarations) because of operator > bool(). > > It's good to minimize the uses of unique_ptr<>::get() so it's easier to reason > about whether there are any stray copies of the pointer. Good point. Didn't realize it had a bool type conversion operator. https://codereview.chromium.org/883673005/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:271: assert(VariableDeclarations.get()); On 2015/02/10 04:01:05, stichnot wrote: > assert(VariableDeclarations); Done. https://codereview.chromium.org/883673005/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:303: return FunctionDeclarationList.size() + Decls->size(); On 2015/02/10 04:01:05, stichnot wrote: > if (VariableDeclarations) { > return FunctionDeclarationList.size() + VariableDeclarations->size(); Done. https://codereview.chromium.org/883673005/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:311: assert(VariableDeclarations.get()); On 2015/02/10 04:01:05, stichnot wrote: > assert(VariableDeclarations); Done. https://codereview.chromium.org/883673005/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:322: return Decls->size(); On 2015/02/10 04:01:04, stichnot wrote: > if (VariableDeclarations) { > return VariableDeclarations->size(); Done. https://codereview.chromium.org/883673005/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:330: assert(VariableDeclarations.get()); On 2015/02/10 04:01:05, stichnot wrote: > assert(VariableDeclarations); Done. https://codereview.chromium.org/883673005/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:348: Ice::VariableDeclarationList *getGlobalVariables() { On 2015/02/10 04:01:04, stichnot wrote: > This would be better returning a unique_ptr. The last statement would be: > > return std::move(VariableDeclarations); I didn't know how to return a unique_ptr, which was why I returned a pointer. Fixing. https://codereview.chromium.org/883673005/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:352: assert(Decls->size() <= ValueIDConstants.size()); On 2015/02/10 04:01:04, stichnot wrote: > if (VariableDeclarations) { > assert(VariableDeclarations->size() <= ...); > > or: > > assert(!VariableDeclarations || VariableDeclarations->size() <= ...); Done. https://codereview.chromium.org/883673005/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:412: void installDeclarationName(// Ice::Translator &Trans, On 2015/02/10 04:01:05, stichnot wrote: > Remove comment in arglist? Same for the two calls below to this function? Done. https://codereview.chromium.org/883673005/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:414: const Ice::IceString &Prefix, const char *Context, On 2015/02/10 04:01:04, stichnot wrote: > Bikeshedding. Maybe rename the Context arg, since there are already a couple of > overloaded uses of this term? Done. https://codereview.chromium.org/883673005/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:426: assert(VariableDeclarations.get()); On 2015/02/10 04:01:04, stichnot wrote: > assert(VariableDeclarations); Done. https://codereview.chromium.org/883673005/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:430: // Ice::Translator &Trans = getTranslator(); On 2015/02/10 04:01:05, stichnot wrote: > remove? Done. https://codereview.chromium.org/883673005/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:443: // Ice::Translator &Trans = getTranslator(); On 2015/02/10 04:01:04, stichnot wrote: > remove? Done. https://codereview.chromium.org/883673005/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:468: for (size_t i = 0; i < NumFunctions; ++i) { On 2015/02/10 04:01:05, stichnot wrote: > Can you use a range-based for loop? I didn't use a range-based loop, since I needed in an earlier version. However, I later realized I needed to set the value using push_back (rather that assignment to array element). Fixing. https://codereview.chromium.org/883673005/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:482: for (size_t i = 0; i < NumGlobalVars; ++i) { On 2015/02/10 04:01:05, stichnot wrote: > Range-based for loop? Done. https://codereview.chromium.org/883673005/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:2872: Context->getGlobalVariables()); On 2015/02/10 04:01:04, stichnot wrote: > With the suggested change to the getGlobalVariable() return type, this would be > something like: > > std::unique_ptr<Ice::VariableDeclarationList> DeclsPtr = > Context->getGlobalVariables(); Done.
otherwise lgtm https://codereview.chromium.org/883673005/diff/60001/src/PNaClTranslator.cpp File src/PNaClTranslator.cpp (right): https://codereview.chromium.org/883673005/diff/60001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:348: std::unique_ptr<Ice::VariableDeclarationList> &&getGlobalVariables() { Does this actually have to have "&&"? I thought it would just be: std::unique_ptr<Ice::VariableDeclarationList> getGlobalVariables() { https://codereview.chromium.org/883673005/diff/60001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:350: // build. built https://codereview.chromium.org/883673005/diff/60001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:408: // NameIndex is used to generate the name. NameIndex is are used https://codereview.chromium.org/883673005/diff/60001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:409: // automatically incremented if a new new is created. "new new" ==> ??? https://codereview.chromium.org/883673005/diff/60001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:411: // created. Mention that it's used just for error reporting? (assuming that's true) https://codereview.chromium.org/883673005/diff/60001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:415: if (!Decl->hasName()) { Rewrite as if (Decl->hasName()) { Translator...; } else { Decl->setName(...); } https://codereview.chromium.org/883673005/diff/60001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:463: for (Ice::FunctionDeclaration *Func : FunctionDeclarationList) { Declare Func as const* if possible. https://codereview.chromium.org/883673005/diff/60001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:475: for (Ice::VariableDeclaration *Decl : *VariableDeclarations) { Also declare Decl as const* if possible.
lgtm https://codereview.chromium.org/883673005/diff/40001/src/PNaClTranslator.cpp File src/PNaClTranslator.cpp (right): https://codereview.chromium.org/883673005/diff/40001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:473: Func->isProto()); Thanks for simplifying the IsExternal a bit. https://codereview.chromium.org/883673005/diff/60001/src/PNaClTranslator.cpp File src/PNaClTranslator.cpp (right): https://codereview.chromium.org/883673005/diff/60001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:351: assert(!VariableDeclarations || Why is it okay for "!VariableDeclarations"? I assume this will only be called once. Is there a use where it's not expected to be initialized?
https://codereview.chromium.org/883673005/diff/60001/src/PNaClTranslator.cpp File src/PNaClTranslator.cpp (right): https://codereview.chromium.org/883673005/diff/60001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:351: assert(!VariableDeclarations || On 2015/02/10 18:24:58, jvoung wrote: > Why is it okay for "!VariableDeclarations"? > > I assume this will only be called once. Is there a use where it's not expected > to be initialized? Hmm, I was under the impression that this could potentially be called multiple times, even though the current implementation only happens to call it once. Whatever the case, I guess it should be documented, probably in IceTranslator.h or .cpp.
https://codereview.chromium.org/883673005/diff/60001/src/PNaClTranslator.cpp File src/PNaClTranslator.cpp (right): https://codereview.chromium.org/883673005/diff/60001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:348: std::unique_ptr<Ice::VariableDeclarationList> &&getGlobalVariables() { On 2015/02/10 17:47:11, stichnot wrote: > Does this actually have to have "&&"? I thought it would just be: > > std::unique_ptr<Ice::VariableDeclarationList> getGlobalVariables() { I thought that would be more efficient, but I'll remove it. https://codereview.chromium.org/883673005/diff/60001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:350: // build. On 2015/02/10 17:47:11, stichnot wrote: > built Done. https://codereview.chromium.org/883673005/diff/60001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:351: assert(!VariableDeclarations || On 2015/02/10 18:29:01, stichnot wrote: > On 2015/02/10 18:24:58, jvoung wrote: > > Why is it okay for "!VariableDeclarations"? > > > > I assume this will only be called once. Is there a use where it's not expected > > to be initialized? > > Hmm, I was under the impression that this could potentially be called multiple > times, even though the current implementation only happens to call it once. > Whatever the case, I guess it should be documented, probably in IceTranslator.h > or .cpp. The code assumes it is only built once. I wanted to make the code do something reasonable if it (in the future) gets called more than once. I assumed that returning nullptr on later calls would allow the user (if it needs to) to know if this was the first call. I updated this comment to communicate this. https://codereview.chromium.org/883673005/diff/60001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:408: // NameIndex is used to generate the name. NameIndex is On 2015/02/10 17:47:11, stichnot wrote: > are used Done. https://codereview.chromium.org/883673005/diff/60001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:409: // automatically incremented if a new new is created. On 2015/02/10 17:47:11, stichnot wrote: > "new new" ==> ??? Fixed. https://codereview.chromium.org/883673005/diff/60001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:411: // created. On 2015/02/10 17:47:11, stichnot wrote: > Mention that it's used just for error reporting? (assuming that's true) This is not true. As stated in the comment, it creates a name if it doesn't have one. However, updated comment to communicate that it adds warning if appropriate. https://codereview.chromium.org/883673005/diff/60001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:415: if (!Decl->hasName()) { On 2015/02/10 17:47:11, stichnot wrote: > Rewrite as > if (Decl->hasName()) { > Translator...; > } else { > Decl->setName(...); > } Done. https://codereview.chromium.org/883673005/diff/60001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:463: for (Ice::FunctionDeclaration *Func : FunctionDeclarationList) { On 2015/02/10 17:47:11, stichnot wrote: > Declare Func as const* if possible. Done. https://codereview.chromium.org/883673005/diff/60001/src/PNaClTranslator.cpp#... src/PNaClTranslator.cpp:475: for (Ice::VariableDeclaration *Decl : *VariableDeclarations) { On 2015/02/10 17:47:11, stichnot wrote: > Also declare Decl as const* if possible. Done.
Message was sent while issue was closed.
Committed patchset #4 (id:60001) manually as 6ca7d2b6bfa9e4bca707240208bba6612d58719d (presubmit successful). |
