Issue 313540043: i#2156. drltrace for malware analysis (Closed)
|
Created:
7 years, 2 months ago by Maxim Shudrak Modified:
7 years, 2 months ago Reviewers:
bruening CC:
dynamorio-devs_googlegroups.com Visibility:
Public. |
DescriptionCommit log for first patchset:
---------------
i#2156: drltrace for malware analysis
Disabled drltrace compilation in case if DynamoRIO
is compiling internaly within DrMemory for Windows.
---------------
Patch Set 1 #
Total comments: 4
Patch Set 2 : removed drltrace #
Total comments: 6
Patch Set 3 : Fixed readme/release.dox #
Total comments: 4
Patch Set 4 : Committed #
MessagesTotal messages: 30
https://codereview.appspot.com/313540043/diff/1/clients/CMakeLists.txt File clients/CMakeLists.txt (right): https://codereview.appspot.com/313540043/diff/1/clients/CMakeLists.txt#newcode75 clients/CMakeLists.txt:75: # enable drltrace for Mac. nit: this comment should remain next to the NOT APPLE line https://codereview.appspot.com/313540043/diff/1/clients/CMakeLists.txt#newcode79 clients/CMakeLists.txt:79: if (NOT DEFINED DRMF_INTERNAL_BUILD AND I don't see this DRMF_INTERNAL_BUILD defined anywhere? The comment should explain that a new drltrace is being built in the DrM repo.
Sign in to reply to this message.
https://codereview.appspot.com/313540043/diff/1/clients/CMakeLists.txt File clients/CMakeLists.txt (right): https://codereview.appspot.com/313540043/diff/1/clients/CMakeLists.txt#newcode79 clients/CMakeLists.txt:79: if (NOT DEFINED DRMF_INTERNAL_BUILD AND On 2017/02/09 15:44:28, bruening wrote: > I don't see this DRMF_INTERNAL_BUILD defined anywhere? All of the existing control over the DR submodule from DrM is done via flags or options that are general within DR: i.e., DR is not aware of DrM. DO_DR_INSTALL, DR_INSTALL_TARGETS_DEST, etc. One argument would be that this should be similar, via some general flag for whether to build drltrace. OTOH if drltrace is being moved, it should just be removed from DR, right? We combine the two in DR release packages so those will still contain it.
Sign in to reply to this message.
On 2017/02/09 16:02:55, bruening wrote: > https://codereview.appspot.com/313540043/diff/1/clients/CMakeLists.txt > File clients/CMakeLists.txt (right): > > https://codereview.appspot.com/313540043/diff/1/clients/CMakeLists.txt#newcode79 > clients/CMakeLists.txt:79: if (NOT DEFINED DRMF_INTERNAL_BUILD AND > On 2017/02/09 15:44:28, bruening wrote: > > I don't see this DRMF_INTERNAL_BUILD defined anywhere? > > All of the existing control over the DR submodule from DrM is done via flags or > options that are general within DR: i.e., DR is not aware of DrM. > DO_DR_INSTALL, DR_INSTALL_TARGETS_DEST, etc. One argument would be that this > should be similar, via some general flag for whether to build drltrace. > > OTOH if drltrace is being moved, it should just be removed from DR, right? We > combine the two in DR release packages so those will still contain it. I'm worry about drltrace for Linux, drltrace for Drmemory is supported only for Windows. So if we remove drltrace from DR do release packages will still contain it ?
Sign in to reply to this message.
On 2017/02/09 16:36:32, Maxim Shudrak wrote: > On 2017/02/09 16:02:55, bruening wrote: > > https://codereview.appspot.com/313540043/diff/1/clients/CMakeLists.txt > > File clients/CMakeLists.txt (right): > > > > > https://codereview.appspot.com/313540043/diff/1/clients/CMakeLists.txt#newcode79 > > clients/CMakeLists.txt:79: if (NOT DEFINED DRMF_INTERNAL_BUILD AND > > On 2017/02/09 15:44:28, bruening wrote: > > > I don't see this DRMF_INTERNAL_BUILD defined anywhere? > > > > All of the existing control over the DR submodule from DrM is done via flags > or > > options that are general within DR: i.e., DR is not aware of DrM. > > DO_DR_INSTALL, DR_INSTALL_TARGETS_DEST, etc. One argument would be that this > > should be similar, via some general flag for whether to build drltrace. > > > > OTOH if drltrace is being moved, it should just be removed from DR, right? We > > combine the two in DR release packages so those will still contain it. > > I'm worry about drltrace for Linux, drltrace for Drmemory is supported only for > Windows. > So if we remove drltrace from DR do release packages will still contain drltrace version for Linux ?
Sign in to reply to this message.
https://codereview.appspot.com/313540043/diff/1/clients/CMakeLists.txt File clients/CMakeLists.txt (right): https://codereview.appspot.com/313540043/diff/1/clients/CMakeLists.txt#newcode79 clients/CMakeLists.txt:79: if (NOT DEFINED DRMF_INTERNAL_BUILD AND On 2017/02/09 15:44:28, bruening wrote: > I don't see this DRMF_INTERNAL_BUILD defined anywhere? > > The comment should explain that a new drltrace is being built in the DrM repo. The DRMF_INTERNAL_BUILD defined in DrMemory's cmake file.
Sign in to reply to this message.
On 2017/02/09 16:39:50, Maxim Shudrak wrote: > On 2017/02/09 16:36:32, Maxim Shudrak wrote: > > On 2017/02/09 16:02:55, bruening wrote: > > > https://codereview.appspot.com/313540043/diff/1/clients/CMakeLists.txt > > > File clients/CMakeLists.txt (right): > > > > > > > > > https://codereview.appspot.com/313540043/diff/1/clients/CMakeLists.txt#newcode79 > > > clients/CMakeLists.txt:79: if (NOT DEFINED DRMF_INTERNAL_BUILD AND > > > On 2017/02/09 15:44:28, bruening wrote: > > > > I don't see this DRMF_INTERNAL_BUILD defined anywhere? > > > > > > All of the existing control over the DR submodule from DrM is done via flags > > or > > > options that are general within DR: i.e., DR is not aware of DrM. > > > DO_DR_INSTALL, DR_INSTALL_TARGETS_DEST, etc. One argument would be that > this > > > should be similar, via some general flag for whether to build drltrace. > > > > > > OTOH if drltrace is being moved, it should just be removed from DR, right? > We > > > combine the two in DR release packages so those will still contain it. > > > > I'm worry about drltrace for Linux, drltrace for Drmemory is supported only > for > > Windows. > > So if we remove drltrace from DR do release packages will still contain > drltrace version for Linux ? What is the reason that drltrace in the DrM repo wouldn't support Linux? Even if there's no Linux library func param database for a while, it would still have today's functionality of just printing 2 args, right?
Sign in to reply to this message.
On 2017/02/09 18:11:26, bruening wrote: > On 2017/02/09 16:39:50, Maxim Shudrak wrote: > > On 2017/02/09 16:36:32, Maxim Shudrak wrote: > > > On 2017/02/09 16:02:55, bruening wrote: > > > > https://codereview.appspot.com/313540043/diff/1/clients/CMakeLists.txt > > > > File clients/CMakeLists.txt (right): > > > > > > > > > > > > > > https://codereview.appspot.com/313540043/diff/1/clients/CMakeLists.txt#newcode79 > > > > clients/CMakeLists.txt:79: if (NOT DEFINED DRMF_INTERNAL_BUILD AND > > > > On 2017/02/09 15:44:28, bruening wrote: > > > > > I don't see this DRMF_INTERNAL_BUILD defined anywhere? > > > > > > > > All of the existing control over the DR submodule from DrM is done via > flags > > > or > > > > options that are general within DR: i.e., DR is not aware of DrM. > > > > DO_DR_INSTALL, DR_INSTALL_TARGETS_DEST, etc. One argument would be that > > this > > > > should be similar, via some general flag for whether to build drltrace. > > > > > > > > OTOH if drltrace is being moved, it should just be removed from DR, right? > > > We > > > > combine the two in DR release packages so those will still contain it. > > > > > > I'm worry about drltrace for Linux, drltrace for Drmemory is supported only > > for > > > Windows. > > > So if we remove drltrace from DR do release packages will still contain > > drltrace version for Linux ? > > What is the reason that drltrace in the DrM repo wouldn't support Linux? Even > if there's no Linux library func param database for a while, it would still have > today's functionality of just printing 2 args, right? Well, ok, I will try to build drltrace for Linux too. It's because I took frontend from drstrace where we have "i#1497, 1498: the drstrace front-end now uses drfrontendlib # but there's a bunch of work getting the clients to be cross-platform."
Sign in to reply to this message.
On 2017/02/09 19:03:07, Maxim Shudrak wrote: > On 2017/02/09 18:11:26, bruening wrote: > > On 2017/02/09 16:39:50, Maxim Shudrak wrote: > > > On 2017/02/09 16:36:32, Maxim Shudrak wrote: > > > > On 2017/02/09 16:02:55, bruening wrote: > > > > > https://codereview.appspot.com/313540043/diff/1/clients/CMakeLists.txt > > > > > File clients/CMakeLists.txt (right): > > > > > > > > > > > > > > > > > > > > https://codereview.appspot.com/313540043/diff/1/clients/CMakeLists.txt#newcode79 > > > > > clients/CMakeLists.txt:79: if (NOT DEFINED DRMF_INTERNAL_BUILD AND > > > > > On 2017/02/09 15:44:28, bruening wrote: > > > > > > I don't see this DRMF_INTERNAL_BUILD defined anywhere? > > > > > > > > > > All of the existing control over the DR submodule from DrM is done via > > flags > > > > or > > > > > options that are general within DR: i.e., DR is not aware of DrM. > > > > > DO_DR_INSTALL, DR_INSTALL_TARGETS_DEST, etc. One argument would be that > > > this > > > > > should be similar, via some general flag for whether to build drltrace. > > > > > > > > > > OTOH if drltrace is being moved, it should just be removed from DR, > right? > > > > > We > > > > > combine the two in DR release packages so those will still contain it. > > > > > > > > I'm worry about drltrace for Linux, drltrace for Drmemory is supported > only > > > for > > > > Windows. > > > > So if we remove drltrace from DR do release packages will still contain > > > drltrace version for Linux ? > > > > What is the reason that drltrace in the DrM repo wouldn't support Linux? Even > > if there's no Linux library func param database for a while, it would still > have > > today's functionality of just printing 2 args, right? > > Well, ok, I will try to build drltrace for Linux too. It's because I took > frontend > from drstrace where we have "i#1497, 1498: the drstrace front-end now uses > drfrontendlib > # but there's a bunch of work getting the clients to be cross-platform." drfrontendlib is cross-platform and is used for Windows, Linux, Mac, and Android in drrun, drcachesim, drmemory, symquery, etc. Those filed issues are mostly about the client, drstracelib. Since the drltrace client is already cross-platform we want to maintain that aspect of it. Having something Windows-only in the DrM repo and similar code that's Linux-only in the DR repo is not appealing.
Sign in to reply to this message.
On 2017/02/09 20:33:38, bruening wrote: > On 2017/02/09 19:03:07, Maxim Shudrak wrote: > > On 2017/02/09 18:11:26, bruening wrote: > > > On 2017/02/09 16:39:50, Maxim Shudrak wrote: > > > > On 2017/02/09 16:36:32, Maxim Shudrak wrote: > > > > > On 2017/02/09 16:02:55, bruening wrote: > > > > > > https://codereview.appspot.com/313540043/diff/1/clients/CMakeLists.txt > > > > > > File clients/CMakeLists.txt (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.appspot.com/313540043/diff/1/clients/CMakeLists.txt#newcode79 > > > > > > clients/CMakeLists.txt:79: if (NOT DEFINED DRMF_INTERNAL_BUILD AND > > > > > > On 2017/02/09 15:44:28, bruening wrote: > > > > > > > I don't see this DRMF_INTERNAL_BUILD defined anywhere? > > > > > > > > > > > > All of the existing control over the DR submodule from DrM is done via > > > flags > > > > > or > > > > > > options that are general within DR: i.e., DR is not aware of DrM. > > > > > > DO_DR_INSTALL, DR_INSTALL_TARGETS_DEST, etc. One argument would be > that > > > > this > > > > > > should be similar, via some general flag for whether to build > drltrace. > > > > > > > > > > > > OTOH if drltrace is being moved, it should just be removed from DR, > > right? > > > > > > > We > > > > > > combine the two in DR release packages so those will still contain it. > > > > > > > > > > I'm worry about drltrace for Linux, drltrace for Drmemory is supported > > only > > > > for > > > > > Windows. > > > > > So if we remove drltrace from DR do release packages will still contain > > > > drltrace version for Linux ? > > > > > > What is the reason that drltrace in the DrM repo wouldn't support Linux? > Even > > > if there's no Linux library func param database for a while, it would still > > have > > > today's functionality of just printing 2 args, right? > > > > Well, ok, I will try to build drltrace for Linux too. It's because I took > > frontend > > from drstrace where we have "i#1497, 1498: the drstrace front-end now uses > > drfrontendlib > > # but there's a bunch of work getting the clients to be cross-platform." > > drfrontendlib is cross-platform and is used for Windows, Linux, Mac, and Android > in drrun, drcachesim, drmemory, symquery, etc. Those filed issues are mostly > about the client, drstracelib. Since the drltrace client is already > cross-platform we want to maintain that aspect of it. Having something > Windows-only in the DrM repo and similar code that's Linux-only in the DR repo > is not appealing. Great, of course.
Sign in to reply to this message.
Commit log for latest patchset: --------------- i#2156. drltrace for malware analysis drltrace removed from dr's dir since we decided to move it into DrMemory. Review-URL: https://codereview.appspot.com/313540043 ---------------
Sign in to reply to this message.
On 2017/02/11 13:05:10, Maxim Shudrak wrote: > Commit log for latest patchset: > --------------- > i#2156. drltrace for malware analysis style: s/./:/
Sign in to reply to this message.
https://codereview.appspot.com/313540043/diff/20001/README.md File README.md (left): https://codereview.appspot.com/313540043/diff/20001/README.md#oldcode30 README.md:30: - The library tracing tool [drltrace](http://dynamorio.org/docs/page_drltrace.html) Given that we have drstrace in this list, it seems like we should keep drltrace here -- so just update the URL to point at drmemory's docs to the forthcoming drltrace docs page, right? https://codereview.appspot.com/313540043/diff/20001/api/docs/release.dox File api/docs/release.dox (right): https://codereview.appspot.com/313540043/diff/20001/api/docs/release.dox#newc... api/docs/release.dox:460: - Added a new page_drltrace See http://dynamorio.org/docs/release_notes.html -- the displayed text was "Added a new Library Tracing Tool". We don't want it to say "page_drltrace". Best to put in "Library Tracing Tool". Also add an entry under the 7.0 changelog (I guess this goes into the 7.0 full release since we don't want to make a branch) that drltrace was moved to Dr. Memory.
Sign in to reply to this message.
Commit log for latest patchset: --------------- i#2156: drltrace for malware analysis drltrace removed from dr's dir since we decided to move the tool to DrMemory. Review-URL: https://codereview.appspot.com/313540043 ---------------
Sign in to reply to this message.
https://codereview.appspot.com/313540043/diff/20001/README.md File README.md (left): https://codereview.appspot.com/313540043/diff/20001/README.md#oldcode30 README.md:30: - The library tracing tool [drltrace](http://dynamorio.org/docs/page_drltrace.html) On 2017/02/13 03:46:56, bruening wrote: > Given that we have drstrace in this list, it seems like we should keep drltrace > here -- so just update the URL to point at drmemory's docs to the forthcoming > drltrace docs page, right? Done. https://codereview.appspot.com/313540043/diff/20001/api/docs/release.dox File api/docs/release.dox (right): https://codereview.appspot.com/313540043/diff/20001/api/docs/release.dox#newc... api/docs/release.dox:460: - Added a new page_drltrace On 2017/02/13 03:46:56, bruening wrote: > See http://dynamorio.org/docs/release_notes.html -- the displayed text was > "Added a new Library Tracing Tool". We don't want it to say "page_drltrace". > Best to put in "Library Tracing Tool". > > Also add an entry under the 7.0 changelog (I guess this goes into the 7.0 full > release since we don't want to make a branch) that drltrace was moved to Dr. > Memory. Done.
Sign in to reply to this message.
Probably you missed that there are two lists -- please leave the first one talking about "compatibility". Else LGTM https://codereview.appspot.com/313540043/diff/20001/api/docs/release.dox File api/docs/release.dox (right): https://codereview.appspot.com/313540043/diff/20001/api/docs/release.dox#newc... api/docs/release.dox:136: following minor compatibility changes: Please leave "compatibility" here as this first list consists of those that break compatibility. The list underneath it is for non-compat changes. Moving drltrace seems disruptive enough to include in the first list. https://codereview.appspot.com/313540043/diff/40001/api/docs/release.dox File api/docs/release.dox (right): https://codereview.appspot.com/313540043/diff/40001/api/docs/release.dox#newc... api/docs/release.dox:138: - drltrace tool has been moved to the Dr.Memory Framework. s/- d/- The d/ https://codereview.appspot.com/313540043/diff/40001/api/docs/release.dox#newc... api/docs/release.dox:461: - Added a new Library Tracing Tool s/Tool/Tool, drltrace./
Sign in to reply to this message.
Committed as https://github.com/DynamoRIO/dynamorio/commit/2d560bce66bd7e071f8ea178d8d516d... Final commit log: --------------- i#2156: drltrace for malware analysis drltrace removed from dr's dir since we decided to move the tool to DrMemory. Review-URL: https://codereview.appspot.com/313540043 ---------------
Sign in to reply to this message.
https://codereview.appspot.com/313540043/diff/20001/api/docs/release.dox File api/docs/release.dox (right): https://codereview.appspot.com/313540043/diff/20001/api/docs/release.dox#newc... api/docs/release.dox:136: following minor compatibility changes: On 2017/02/13 21:33:11, bruening wrote: > Please leave "compatibility" here as this first list consists of those that > break compatibility. The list underneath it is for non-compat changes. Moving > drltrace seems disruptive enough to include in the first list. Done. https://codereview.appspot.com/313540043/diff/40001/api/docs/release.dox File api/docs/release.dox (right): https://codereview.appspot.com/313540043/diff/40001/api/docs/release.dox#newc... api/docs/release.dox:138: - drltrace tool has been moved to the Dr.Memory Framework. On 2017/02/13 21:33:11, bruening wrote: > s/- d/- The d/ Done. https://codereview.appspot.com/313540043/diff/40001/api/docs/release.dox#newc... api/docs/release.dox:461: - Added a new Library Tracing Tool On 2017/02/13 21:33:11, bruening wrote: > s/Tool/Tool, drltrace./ Done.
Sign in to reply to this message.
On 2017/02/14 17:46:27, Maxim Shudrak wrote: > https://codereview.appspot.com/313540043/diff/20001/api/docs/release.dox > File api/docs/release.dox (right): > > https://codereview.appspot.com/313540043/diff/20001/api/docs/release.dox#newc... > api/docs/release.dox:136: following minor compatibility changes: > On 2017/02/13 21:33:11, bruening wrote: > > Please leave "compatibility" here as this first list consists of those that > > break compatibility. The list underneath it is for non-compat changes. > Moving > > drltrace seems disruptive enough to include in the first list. > > Done. > > https://codereview.appspot.com/313540043/diff/40001/api/docs/release.dox > File api/docs/release.dox (right): > > https://codereview.appspot.com/313540043/diff/40001/api/docs/release.dox#newc... > api/docs/release.dox:138: - drltrace tool has been moved to the Dr.Memory > Framework. > On 2017/02/13 21:33:11, bruening wrote: > > s/- d/- The d/ > > Done. > > https://codereview.appspot.com/313540043/diff/40001/api/docs/release.dox#newc... > api/docs/release.dox:461: - Added a new Library Tracing Tool > On 2017/02/13 21:33:11, bruening wrote: > > s/Tool/Tool, drltrace./ > > Done. It seems that I again have a problem with pushing my changes on the github. I did git pullall and pass through the tests. Than I made git dcommit. At the same time it turns that there was a new commit (while I did tests), so the issue on rietvield has been closed but git raised an error. How can I re-associate my latest changes with this Rietveld issue ?
Sign in to reply to this message.
On 2017/02/14 18:08:03, Maxim Shudrak wrote: > On 2017/02/14 17:46:27, Maxim Shudrak wrote: > > https://codereview.appspot.com/313540043/diff/20001/api/docs/release.dox > > File api/docs/release.dox (right): > > > > > https://codereview.appspot.com/313540043/diff/20001/api/docs/release.dox#newc... > > api/docs/release.dox:136: following minor compatibility changes: > > On 2017/02/13 21:33:11, bruening wrote: > > > Please leave "compatibility" here as this first list consists of those that > > > break compatibility. The list underneath it is for non-compat changes. > > Moving > > > drltrace seems disruptive enough to include in the first list. > > > > Done. > > > > https://codereview.appspot.com/313540043/diff/40001/api/docs/release.dox > > File api/docs/release.dox (right): > > > > > https://codereview.appspot.com/313540043/diff/40001/api/docs/release.dox#newc... > > api/docs/release.dox:138: - drltrace tool has been moved to the Dr.Memory > > Framework. > > On 2017/02/13 21:33:11, bruening wrote: > > > s/- d/- The d/ > > > > Done. > > > > > https://codereview.appspot.com/313540043/diff/40001/api/docs/release.dox#newc... > > api/docs/release.dox:461: - Added a new Library Tracing Tool > > On 2017/02/13 21:33:11, bruening wrote: > > > s/Tool/Tool, drltrace./ > > > > Done. > > It seems that I again have a problem with pushing my changes on the github. I > did git pullall and pass through the tests. > Than I made git dcommit. At the same time it turns that there was a new commit > (while I did tests), so the issue on rietvield has been closed but git raised an > error. > How can I re-associate my latest changes with this Rietveld issue ? If it wasn't pushed, isn't the Review-URL line still in your commit message? Do a "git push origin HEAD:master".
Sign in to reply to this message.
On 2017/02/14 18:28:28, bruening wrote: > On 2017/02/14 18:08:03, Maxim Shudrak wrote: > > On 2017/02/14 17:46:27, Maxim Shudrak wrote: > > > https://codereview.appspot.com/313540043/diff/20001/api/docs/release.dox > > > File api/docs/release.dox (right): > > > > > > > > > https://codereview.appspot.com/313540043/diff/20001/api/docs/release.dox#newc... > > > api/docs/release.dox:136: following minor compatibility changes: > > > On 2017/02/13 21:33:11, bruening wrote: > > > > Please leave "compatibility" here as this first list consists of those > that > > > > break compatibility. The list underneath it is for non-compat changes. > > > Moving > > > > drltrace seems disruptive enough to include in the first list. > > > > > > Done. > > > > > > https://codereview.appspot.com/313540043/diff/40001/api/docs/release.dox > > > File api/docs/release.dox (right): > > > > > > > > > https://codereview.appspot.com/313540043/diff/40001/api/docs/release.dox#newc... > > > api/docs/release.dox:138: - drltrace tool has been moved to the Dr.Memory > > > Framework. > > > On 2017/02/13 21:33:11, bruening wrote: > > > > s/- d/- The d/ > > > > > > Done. > > > > > > > > > https://codereview.appspot.com/313540043/diff/40001/api/docs/release.dox#newc... > > > api/docs/release.dox:461: - Added a new Library Tracing Tool > > > On 2017/02/13 21:33:11, bruening wrote: > > > > s/Tool/Tool, drltrace./ > > > > > > Done. > > > > It seems that I again have a problem with pushing my changes on the github. I > > did git pullall and pass through the tests. > > Than I made git dcommit. At the same time it turns that there was a new commit > > (while I did tests), so the issue on rietvield has been closed but git raised > an > > error. > > How can I re-associate my latest changes with this Rietveld issue ? > > If it wasn't pushed, isn't the Review-URL line still in your commit message? Do > a "git push origin HEAD:master". Note that we are moving away from Rietveld toward using Github pull requests on repo branches. The precise workflow has not yet been nailed down though nor put into the wiki.
Sign in to reply to this message.
On 2017/02/14 18:29:57, bruening wrote: > On 2017/02/14 18:28:28, bruening wrote: > > On 2017/02/14 18:08:03, Maxim Shudrak wrote: > > > On 2017/02/14 17:46:27, Maxim Shudrak wrote: > > > > https://codereview.appspot.com/313540043/diff/20001/api/docs/release.dox > > > > File api/docs/release.dox (right): > > > > > > > > > > > > > > https://codereview.appspot.com/313540043/diff/20001/api/docs/release.dox#newc... > > > > api/docs/release.dox:136: following minor compatibility changes: > > > > On 2017/02/13 21:33:11, bruening wrote: > > > > > Please leave "compatibility" here as this first list consists of those > > that > > > > > break compatibility. The list underneath it is for non-compat changes. > > > > Moving > > > > > drltrace seems disruptive enough to include in the first list. > > > > > > > > Done. > > > > > > > > https://codereview.appspot.com/313540043/diff/40001/api/docs/release.dox > > > > File api/docs/release.dox (right): > > > > > > > > > > > > > > https://codereview.appspot.com/313540043/diff/40001/api/docs/release.dox#newc... > > > > api/docs/release.dox:138: - drltrace tool has been moved to the Dr.Memory > > > > Framework. > > > > On 2017/02/13 21:33:11, bruening wrote: > > > > > s/- d/- The d/ > > > > > > > > Done. > > > > > > > > > > > > > > https://codereview.appspot.com/313540043/diff/40001/api/docs/release.dox#newc... > > > > api/docs/release.dox:461: - Added a new Library Tracing Tool > > > > On 2017/02/13 21:33:11, bruening wrote: > > > > > s/Tool/Tool, drltrace./ > > > > > > > > Done. > > > > > > It seems that I again have a problem with pushing my changes on the github. > I > > > did git pullall and pass through the tests. > > > Than I made git dcommit. At the same time it turns that there was a new > commit > > > (while I did tests), so the issue on rietvield has been closed but git > raised > > an > > > error. > > > How can I re-associate my latest changes with this Rietveld issue ? > > > > If it wasn't pushed, isn't the Review-URL line still in your commit message? > Do > > a "git push origin HEAD:master". > > Note that we are moving away from Rietveld toward using Github pull requests on > repo branches. The precise workflow has not yet been nailed down though nor put > into the wiki. Yes, I tried git push origin HEAD:master, have the following errors: remote: Resolving deltas: 100% (8/8), completed with 8 local objects. remote: error: GH006: Protected branch update failed for refs/heads/master. remote: error: At least one approved review is required
Sign in to reply to this message.
On Tue, Feb 14, 2017 at 1:33 PM, <mxmssh@gmail.com> wrote: > Yes, I tried git push origin HEAD:master, have the following errors: > remote: Resolving deltas: 100% (8/8), completed with 8 local objects. > remote: error: GH006: Protected branch update failed for > refs/heads/master. > remote: error: At least one approved review is required Hmm, so the settings I put in for pull requests affect direct pushes too. I just relaxed them so you should be able to push now.
Sign in to reply to this message.
On 2017/02/14 18:08:03, Maxim Shudrak wrote: > On 2017/02/14 17:46:27, Maxim Shudrak wrote: > > https://codereview.appspot.com/313540043/diff/20001/api/docs/release.dox > > File api/docs/release.dox (right): > > > > > https://codereview.appspot.com/313540043/diff/20001/api/docs/release.dox#newc... > > api/docs/release.dox:136: following minor compatibility changes: > > On 2017/02/13 21:33:11, bruening wrote: > > > Please leave "compatibility" here as this first list consists of those that > > > break compatibility. The list underneath it is for non-compat changes. > > Moving > > > drltrace seems disruptive enough to include in the first list. > > > > Done. > > > > https://codereview.appspot.com/313540043/diff/40001/api/docs/release.dox > > File api/docs/release.dox (right): > > > > > https://codereview.appspot.com/313540043/diff/40001/api/docs/release.dox#newc... > > api/docs/release.dox:138: - drltrace tool has been moved to the Dr.Memory > > Framework. > > On 2017/02/13 21:33:11, bruening wrote: > > > s/- d/- The d/ > > > > Done. > > > > > https://codereview.appspot.com/313540043/diff/40001/api/docs/release.dox#newc... > > api/docs/release.dox:461: - Added a new Library Tracing Tool > > On 2017/02/13 21:33:11, bruening wrote: > > > s/Tool/Tool, drltrace./ > > > > Done. > > It seems that I again have a problem with pushing my changes on the github. I > did git pullall and pass through the tests. > Than I made git dcommit. At the same time it turns that there was a new commit > (while I did tests), so the issue on rietvield has been closed but git raised an > error. > How can I re-associate my latest changes with this Rietveld issue ? I can create a new one if it is ok.
Sign in to reply to this message.
On 2017/02/14 18:58:33, Maxim Shudrak wrote: > On 2017/02/14 18:08:03, Maxim Shudrak wrote: > > On 2017/02/14 17:46:27, Maxim Shudrak wrote: > > > https://codereview.appspot.com/313540043/diff/20001/api/docs/release.dox > > > File api/docs/release.dox (right): > > > > > > > > > https://codereview.appspot.com/313540043/diff/20001/api/docs/release.dox#newc... > > > api/docs/release.dox:136: following minor compatibility changes: > > > On 2017/02/13 21:33:11, bruening wrote: > > > > Please leave "compatibility" here as this first list consists of those > that > > > > break compatibility. The list underneath it is for non-compat changes. > > > Moving > > > > drltrace seems disruptive enough to include in the first list. > > > > > > Done. > > > > > > https://codereview.appspot.com/313540043/diff/40001/api/docs/release.dox > > > File api/docs/release.dox (right): > > > > > > > > > https://codereview.appspot.com/313540043/diff/40001/api/docs/release.dox#newc... > > > api/docs/release.dox:138: - drltrace tool has been moved to the Dr.Memory > > > Framework. > > > On 2017/02/13 21:33:11, bruening wrote: > > > > s/- d/- The d/ > > > > > > Done. > > > > > > > > > https://codereview.appspot.com/313540043/diff/40001/api/docs/release.dox#newc... > > > api/docs/release.dox:461: - Added a new Library Tracing Tool > > > On 2017/02/13 21:33:11, bruening wrote: > > > > s/Tool/Tool, drltrace./ > > > > > > Done. > > > > It seems that I again have a problem with pushing my changes on the github. I > > did git pullall and pass through the tests. > > Than I made git dcommit. At the same time it turns that there was a new commit > > (while I did tests), so the issue on rietvield has been closed but git raised > an > > error. > > How can I re-associate my latest changes with this Rietveld issue ? > > I can create a new one if it is ok. Sure, I just updated https://github.com/DynamoRIO/dynamorio/wiki/Workflow and https://github.com/DynamoRIO/dynamorio/wiki/Code-Reviews. Please see whether they make sense and try it out.
Sign in to reply to this message.
On 2017/02/14 20:01:22, bruening wrote: > On 2017/02/14 18:58:33, Maxim Shudrak wrote: > > On 2017/02/14 18:08:03, Maxim Shudrak wrote: > > > On 2017/02/14 17:46:27, Maxim Shudrak wrote: > > > > https://codereview.appspot.com/313540043/diff/20001/api/docs/release.dox > > > > File api/docs/release.dox (right): > > > > > > > > > > > > > > https://codereview.appspot.com/313540043/diff/20001/api/docs/release.dox#newc... > > > > api/docs/release.dox:136: following minor compatibility changes: > > > > On 2017/02/13 21:33:11, bruening wrote: > > > > > Please leave "compatibility" here as this first list consists of those > > that > > > > > break compatibility. The list underneath it is for non-compat changes. > > > > Moving > > > > > drltrace seems disruptive enough to include in the first list. > > > > > > > > Done. > > > > > > > > https://codereview.appspot.com/313540043/diff/40001/api/docs/release.dox > > > > File api/docs/release.dox (right): > > > > > > > > > > > > > > https://codereview.appspot.com/313540043/diff/40001/api/docs/release.dox#newc... > > > > api/docs/release.dox:138: - drltrace tool has been moved to the Dr.Memory > > > > Framework. > > > > On 2017/02/13 21:33:11, bruening wrote: > > > > > s/- d/- The d/ > > > > > > > > Done. > > > > > > > > > > > > > > https://codereview.appspot.com/313540043/diff/40001/api/docs/release.dox#newc... > > > > api/docs/release.dox:461: - Added a new Library Tracing Tool > > > > On 2017/02/13 21:33:11, bruening wrote: > > > > > s/Tool/Tool, drltrace./ > > > > > > > > Done. > > > > > > It seems that I again have a problem with pushing my changes on the github. > I > > > did git pullall and pass through the tests. > > > Than I made git dcommit. At the same time it turns that there was a new > commit > > > (while I did tests), so the issue on rietvield has been closed but git > raised > > an > > > error. > > > How can I re-associate my latest changes with this Rietveld issue ? > > > > I can create a new one if it is ok. > > Sure, I just updated https://github.com/DynamoRIO/dynamorio/wiki/Workflow and > https://github.com/DynamoRIO/dynamorio/wiki/Code-Reviews. Please see whether > they make sense and try it out. Cool, this workflow looks easier, I have created a pull request several minutes ago. Did you get any message ?
Sign in to reply to this message.
On 2017/02/15 17:32:14, Maxim Shudrak wrote: > Cool, this workflow looks easier, I have created a pull request several minutes > ago. > Did you get any message ? Yes, should get to it shortly.
Sign in to reply to this message.
On 2017/02/15 17:32:14, Maxim Shudrak wrote: > On 2017/02/14 20:01:22, bruening wrote: > > On 2017/02/14 18:58:33, Maxim Shudrak wrote: > > > On 2017/02/14 18:08:03, Maxim Shudrak wrote: > > > > On 2017/02/14 17:46:27, Maxim Shudrak wrote: > > > > > https://codereview.appspot.com/313540043/diff/20001/api/docs/release.dox > > > > > File api/docs/release.dox (right): > > > > > > > > > > > > > > > > > > > > https://codereview.appspot.com/313540043/diff/20001/api/docs/release.dox#newc... > > > > > api/docs/release.dox:136: following minor compatibility changes: > > > > > On 2017/02/13 21:33:11, bruening wrote: > > > > > > Please leave "compatibility" here as this first list consists of those > > > that > > > > > > break compatibility. The list underneath it is for non-compat > changes. > > > > > Moving > > > > > > drltrace seems disruptive enough to include in the first list. > > > > > > > > > > Done. > > > > > > > > > > https://codereview.appspot.com/313540043/diff/40001/api/docs/release.dox > > > > > File api/docs/release.dox (right): > > > > > > > > > > > > > > > > > > > > https://codereview.appspot.com/313540043/diff/40001/api/docs/release.dox#newc... > > > > > api/docs/release.dox:138: - drltrace tool has been moved to the > Dr.Memory > > > > > Framework. > > > > > On 2017/02/13 21:33:11, bruening wrote: > > > > > > s/- d/- The d/ > > > > > > > > > > Done. > > > > > > > > > > > > > > > > > > > > https://codereview.appspot.com/313540043/diff/40001/api/docs/release.dox#newc... > > > > > api/docs/release.dox:461: - Added a new Library Tracing Tool > > > > > On 2017/02/13 21:33:11, bruening wrote: > > > > > > s/Tool/Tool, drltrace./ > > > > > > > > > > Done. > > > > > > > > It seems that I again have a problem with pushing my changes on the > github. > > I > > > > did git pullall and pass through the tests. > > > > Than I made git dcommit. At the same time it turns that there was a new > > commit > > > > (while I did tests), so the issue on rietvield has been closed but git > > raised > > > an > > > > error. > > > > How can I re-associate my latest changes with this Rietveld issue ? > > > > > > I can create a new one if it is ok. > > > > Sure, I just updated https://github.com/DynamoRIO/dynamorio/wiki/Workflow and > > https://github.com/DynamoRIO/dynamorio/wiki/Code-Reviews. Please see whether > > they make sense and try it out. > > Cool, this workflow looks easier, I have created a pull request several minutes > ago. > Did you get any message ? What if I would need to commit a second patchset ? Does github support this ?
Sign in to reply to this message.
On 2017/02/15 17:36:38, Maxim Shudrak wrote: > What if I would need to commit a second patchset ? Does github support this ? See https://github.com/DynamoRIO/dynamorio/wiki/Code-Reviews under "Responding to Review Comments"
Sign in to reply to this message.
On 2017/02/15 17:49:27, bruening wrote: > On 2017/02/15 17:36:38, Maxim Shudrak wrote: > > What if I would need to commit a second patchset ? Does github support this ? > > See https://github.com/DynamoRIO/dynamorio/wiki/Code-Reviews under "Responding > to Review Comments" Ok, thank you.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
