Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
886563002:
Switch from nm to objdump for the cygprofile tools. (Closed)
DescriptionSwitch from nm to objdump for the cygprofile tools.
This is required for further refactoring (use in symbolize.py), as nm
doesn't provide the section name.
Also, some functions in symbol_extractor.py are not
used yet, they will in the next CLs of the refactoring.
BUG=452879
Committed: https://crrev.com/737b1473709df32f82adc8bb6654927aad25b7d9
Cr-Commit-Position: refs/heads/master@{#313715}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Address comments. #
Total comments: 13
Patch Set 3 : Address comments. #
Total comments: 2
Patch Set 4 : Address comments. #
Total comments: 6
Patch Set 5 : Address comments. #Patch Set 6 : Typo. #
Messages
Total messages: 14 (2 generated)
lizeb@chromium.org changed reviewers: + pasko@chromium.org
https://codereview.chromium.org/886563002/diff/1/tools/cygprofile/patch_order... File tools/cygprofile/patch_orderfile_unittest.py (right): https://codereview.chromium.org/886563002/diff/1/tools/cygprofile/patch_order... tools/cygprofile/patch_orderfile_unittest.py:25: section='.text')] nit: some more whitespace here and below https://codereview.chromium.org/886563002/diff/1/tools/cygprofile/symbol_extr... File tools/cygprofile/symbol_extractor.py (right): https://codereview.chromium.org/886563002/diff/1/tools/cygprofile/symbol_extr... tools/cygprofile/symbol_extractor.py:22: _OBJDUMP_BINARY = symbol.ToolPath('objdump') The phrase 'This is required, as nm doesn't provide the section name' confuses me. The 'section' value provided by _FromObjdumpLine() would be either '.text' or '*UND*', so this CL won't change the orderfile. I am guessing this is needed to be able to use the same SymbolInfo when we parse the lines from the objects (*.o files)? If we really want to share the 8 lines of code for objdump parsing, let's create an argument, say, extract_section_name=True|False. Though it's not clear what the plan is. I imagined we would make a global 'symbol to section map', just like we do today, and use it to update every SymbolInfo we parsed from the DSO (libchrome.so). Updating symbol.name would be just fine for these purposes. Please clarify. https://codereview.chromium.org/886563002/diff/1/tools/cygprofile/symbol_extr... tools/cygprofile/symbol_extractor.py:60: # TODO(lizeb): Consider switching to objdump to simplify parsing. this change is implementing the TODO (!)
https://codereview.chromium.org/886563002/diff/1/tools/cygprofile/patch_order... File tools/cygprofile/patch_orderfile_unittest.py (right): https://codereview.chromium.org/886563002/diff/1/tools/cygprofile/patch_order... tools/cygprofile/patch_orderfile_unittest.py:25: section='.text')] On 2015/01/28 18:55:37, pasko wrote: > nit: some more whitespace here and below Done. https://codereview.chromium.org/886563002/diff/1/tools/cygprofile/symbol_extr... File tools/cygprofile/symbol_extractor.py (right): https://codereview.chromium.org/886563002/diff/1/tools/cygprofile/symbol_extr... tools/cygprofile/symbol_extractor.py:22: _OBJDUMP_BINARY = symbol.ToolPath('objdump') On 2015/01/28 18:55:37, pasko wrote: > The phrase 'This is required, as nm doesn't provide the section name' confuses > me. > > The 'section' value provided by _FromObjdumpLine() would be either '.text' or > '*UND*', so this CL won't change the orderfile. I am guessing this is needed to > be able to use the same SymbolInfo when we parse the lines from the objects (*.o > files)? If we really want to share the 8 lines of code for objdump parsing, > let's create an argument, say, extract_section_name=True|False. > > Though it's not clear what the plan is. I imagined we would make a global > 'symbol to section map', just like we do today, and use it to update every > SymbolInfo we parsed from the DSO (libchrome.so). Updating symbol.name would be > just fine for these purposes. > > Please clarify. Yes, the goal is to have only one way to parse object files, and the purpose of this CL is to make the parsing sufficiently rich for symbolize.py. The section indeed matters only for the symbolize.py use, that is parsing object files. Since it is not a problem elsewhere, let's kep only one code path, and less arguments. What do you think? https://codereview.chromium.org/886563002/diff/1/tools/cygprofile/symbol_extr... tools/cygprofile/symbol_extractor.py:60: # TODO(lizeb): Consider switching to objdump to simplify parsing. On 2015/01/28 18:55:37, pasko wrote: > this change is implementing the TODO (!) Arg, indeed. It is so rare to remove TODOs, it would be bad to forget one that is done. Done.
OK, overall the change will help us iterate faster, thank you https://codereview.chromium.org/886563002/diff/1/tools/cygprofile/symbol_extr... File tools/cygprofile/symbol_extractor.py (right): https://codereview.chromium.org/886563002/diff/1/tools/cygprofile/symbol_extr... tools/cygprofile/symbol_extractor.py:22: _OBJDUMP_BINARY = symbol.ToolPath('objdump') On 2015/01/29 10:02:01, lizeb wrote: > On 2015/01/28 18:55:37, pasko wrote: > > The phrase 'This is required, as nm doesn't provide the section name' confuses > > me. > > > > The 'section' value provided by _FromObjdumpLine() would be either '.text' or > > '*UND*', so this CL won't change the orderfile. I am guessing this is needed > to > > be able to use the same SymbolInfo when we parse the lines from the objects > (*.o > > files)? If we really want to share the 8 lines of code for objdump parsing, > > let's create an argument, say, extract_section_name=True|False. > > > > Though it's not clear what the plan is. I imagined we would make a global > > 'symbol to section map', just like we do today, and use it to update every > > SymbolInfo we parsed from the DSO (libchrome.so). Updating symbol.name would > be > > just fine for these purposes. > > > > Please clarify. > > Yes, the goal is to have only one way to parse object files, and the purpose of > this CL is to make the parsing sufficiently rich for symbolize.py. > > The section indeed matters only for the symbolize.py use, that is parsing object > files. Since it is not a problem elsewhere, let's kep only one code path, and > less arguments. > > What do you think? Sounds good, but please either remove the 'required' part from the CL description or enhance it to say that it is required to share the parsing logic with symbolize.py https://codereview.chromium.org/886563002/diff/1/tools/cygprofile/symbol_extr... tools/cygprofile/symbol_extractor.py:44: offset = int(parts[0], 16) To prevent loosing information without knowing let's put some assertions here: assert(len(parts) == 5 or (len(parts) == 5 and parts[5] == '.hidden')) accepted_scopes = set(['g', 'l', 'w']) assert(parts[1] in accepted_scopes) also, it would be nice to check that the symbol and section name are in [:alnum:] + underscores. btw, did you compare your orderfile patching locally before/after the refactoring? https://codereview.chromium.org/886563002/diff/1/tools/cygprofile/symbol_extr... tools/cygprofile/symbol_extractor.py:60: # TODO(lizeb): Consider switching to objdump to simplify parsing. On 2015/01/29 10:02:01, lizeb wrote: > On 2015/01/28 18:55:37, pasko wrote: > > this change is implementing the TODO (!) > > Arg, indeed. It is so rare to remove TODOs, it would be bad to forget one that > is done. > Done. the blame is on me for nagging to add this TODO, thanks for reminding about that, I almost forgot, but no, things like that must not be forgotten .. I frankly did not expect it to arrive that fast, it's the finding about the 'nm' tool taken from an incorrect place boosted the interest in revamping tool usage. Thanks for doing! https://codereview.chromium.org/886563002/diff/20001/tools/cygprofile/symbol_... File tools/cygprofile/symbol_extractor.py (right): https://codereview.chromium.org/886563002/diff/20001/tools/cygprofile/symbol_... tools/cygprofile/symbol_extractor.py:15: 0, os.path.join(sys.path[0], '..', '..', 'third_party', 'android_platform', why not sys.path.append()? let's also not make any assumption about where the scripts is invoked from, please do it similarly to: tools/android/checkstyle/checkstyle.py https://codereview.chromium.org/886563002/diff/20001/tools/cygprofile/symbol_... tools/cygprofile/symbol_extractor.py:104: def CreateNameToSymbolInfo(symbol_infos): this function is only used in a test, is it going to be useful in a following CL? If so, please add it there. Dead code is easy to forget about, you know. https://codereview.chromium.org/886563002/diff/20001/tools/cygprofile/symbol_... File tools/cygprofile/symbol_extractor_unittest.py (right): https://codereview.chromium.org/886563002/diff/20001/tools/cygprofile/symbol_... tools/cygprofile/symbol_extractor_unittest.py:15: # Too short All our styleguides strongly recommend the comments that occupy their lines would contain a sequence of _sentences_. Sentences must start from a capital letter and end with a period. This one should say something like: # The following line is too short for the parser. or just: # This line is too short. https://codereview.chromium.org/886563002/diff/20001/tools/cygprofile/symbol_... tools/cygprofile/symbol_extractor_unittest.py:40: 'hidden _GLOBAL__sub_I_chrome_main_delegate.cc') s/hidden/.hidden/ I <3 realism. https://codereview.chromium.org/886563002/diff/20001/tools/cygprofile/symbol_... tools/cygprofile/symbol_extractor_unittest.py:52: class TestSymbolInfosFromStream(unittest.TestCase): more whitespace is absolutely necessary, otherwise human parsers become even less reliable. I am wondering why pyling does not complain about these :) https://codereview.chromium.org/886563002/diff/20001/tools/cygprofile/symbol_... tools/cygprofile/symbol_extractor_unittest.py:85: def testCreateNameToSymbolInfos(self): nittish nit: if this test is to stay, probably should be named testCreateNameToSymbolInfo (without the trailing 's'), to suggest what function it is testing, and less to suggest what it's doing.
Thank you for the review. All done. https://codereview.chromium.org/886563002/diff/20001/tools/cygprofile/symbol_... File tools/cygprofile/symbol_extractor.py (right): https://codereview.chromium.org/886563002/diff/20001/tools/cygprofile/symbol_... tools/cygprofile/symbol_extractor.py:15: 0, os.path.join(sys.path[0], '..', '..', 'third_party', 'android_platform', On 2015/01/29 12:24:42, pasko wrote: > why not sys.path.append()? > > let's also not make any assumption about where the scripts is invoked from, > please do it similarly to: > tools/android/checkstyle/checkstyle.py Because python's standard library also has a module named "symbol". And it is not possible to import the parent directory, as it lacks the __init__.py file required to make it importable. The same mechanism is used in asan_symbolize.py. Done for the path. https://codereview.chromium.org/886563002/diff/20001/tools/cygprofile/symbol_... tools/cygprofile/symbol_extractor.py:104: def CreateNameToSymbolInfo(symbol_infos): On 2015/01/29 12:24:42, pasko wrote: > this function is only used in a test, is it going to be useful in a following > CL? If so, please add it there. Dead code is easy to forget about, you know. Yes, it is going to be used. But I would prefer to separate the introduction of common functions from the refactoring making use of them. Especially since this method is tested. What do you think? https://codereview.chromium.org/886563002/diff/20001/tools/cygprofile/symbol_... File tools/cygprofile/symbol_extractor_unittest.py (right): https://codereview.chromium.org/886563002/diff/20001/tools/cygprofile/symbol_... tools/cygprofile/symbol_extractor_unittest.py:15: # Too short On 2015/01/29 12:24:43, pasko wrote: > All our styleguides strongly recommend the comments that occupy their lines > would contain a sequence of _sentences_. Sentences must start from a capital > letter and end with a period. > > This one should say something like: > # The following line is too short for the parser. > > or just: > # This line is too short. Done. https://codereview.chromium.org/886563002/diff/20001/tools/cygprofile/symbol_... tools/cygprofile/symbol_extractor_unittest.py:40: 'hidden _GLOBAL__sub_I_chrome_main_delegate.cc') On 2015/01/29 12:24:42, pasko wrote: > s/hidden/.hidden/ > > I <3 realism. Done. https://codereview.chromium.org/886563002/diff/20001/tools/cygprofile/symbol_... tools/cygprofile/symbol_extractor_unittest.py:52: class TestSymbolInfosFromStream(unittest.TestCase): On 2015/01/29 12:24:42, pasko wrote: > more whitespace is absolutely necessary, otherwise human parsers become even > less reliable. I am wondering why pyling does not complain about these :) Done. https://codereview.chromium.org/886563002/diff/20001/tools/cygprofile/symbol_... tools/cygprofile/symbol_extractor_unittest.py:85: def testCreateNameToSymbolInfos(self): On 2015/01/29 12:24:43, pasko wrote: > nittish nit: > > if this test is to stay, probably should be named testCreateNameToSymbolInfo > (without the trailing 's'), to suggest what function it is testing, and less to > suggest what it's doing. Done.
not really "all" was done. Please don't forget to update the issue description. https://codereview.chromium.org/886563002/diff/20001/tools/cygprofile/symbol_... File tools/cygprofile/symbol_extractor.py (right): https://codereview.chromium.org/886563002/diff/20001/tools/cygprofile/symbol_... tools/cygprofile/symbol_extractor.py:104: def CreateNameToSymbolInfo(symbol_infos): On 2015/01/29 13:10:30, lizeb wrote: > On 2015/01/29 12:24:42, pasko wrote: > > this function is only used in a test, is it going to be useful in a following > > CL? If so, please add it there. Dead code is easy to forget about, you know. > > Yes, it is going to be used. But I would prefer to separate the introduction of > common functions from the refactoring making use of them. Especially since this > method is tested. > > What do you think? since it confuses reviewers, that's usually worth an extra comment in the commit description. So you are free to choose, whether to explain in the commit description that the plan is to use this and that function in a coming-soon CL, or just introduce things lazily when needed. I prefer the 2nd approach, because it's less to write/explain. https://codereview.chromium.org/886563002/diff/40001/tools/cygprofile/symbol_... File tools/cygprofile/symbol_extractor.py (right): https://codereview.chromium.org/886563002/diff/40001/tools/cygprofile/symbol_... tools/cygprofile/symbol_extractor.py:45: offset = int(parts[0], 16) I think one of my previous comments was lost, I don't see it in my PatchSet2, but it was emailed and is there in comments #5: To prevent loosing information without knowing let's put some assertions here: assert(len(parts) == 5 or (len(parts) == 5 and parts[5] == '.hidden')) accepted_scopes = set(['g', 'l', 'w']) assert(parts[1] in accepted_scopes) also, it would be nice to check that the symbol and section name are in [:alnum:] + underscores. btw, did you compare your orderfile patching locally before/after the refactoring?
https://codereview.chromium.org/886563002/diff/40001/tools/cygprofile/symbol_... File tools/cygprofile/symbol_extractor.py (right): https://codereview.chromium.org/886563002/diff/40001/tools/cygprofile/symbol_... tools/cygprofile/symbol_extractor.py:45: offset = int(parts[0], 16) On 2015/01/29 13:25:30, pasko wrote: > I think one of my previous comments was lost, I don't see it in my PatchSet2, > but it was emailed and is there in comments #5: > > To prevent loosing information without knowing let's put some assertions > here: > assert(len(parts) == 5 or (len(parts) == 5 and parts[5] == '.hidden')) > accepted_scopes = set(['g', 'l', 'w']) > assert(parts[1] in accepted_scopes) > > also, it would be nice to check that the symbol and section name are in > [:alnum:] + underscores. > > btw, did you compare your orderfile patching locally before/after the > refactoring? Done.
lgtm with a few changes in tests https://codereview.chromium.org/886563002/diff/60001/tools/cygprofile/symbol_... File tools/cygprofile/symbol_extractor_unittest.py (right): https://codereview.chromium.org/886563002/diff/60001/tools/cygprofile/symbol_... tools/cygprofile/symbol_extractor_unittest.py:25: # This line has an invalid scope period? here and below please, if you will https://codereview.chromium.org/886563002/diff/60001/tools/cygprofile/symbol_... tools/cygprofile/symbol_extractor_unittest.py:27: self.assertRaises(AssertionError, symbol_extractor._FromObjdumpLine, line) omg this is so cool! I did not know about it, and now absolutely love it. https://codereview.chromium.org/886563002/diff/60001/tools/cygprofile/symbol_... tools/cygprofile/symbol_extractor_unittest.py:29: line = ('00c1b228 z F .text 00000060 _ZN20trace_event too many') this asserts for the same reason as the previous one, so: s/ z / l / same below
Thank you ! https://codereview.chromium.org/886563002/diff/60001/tools/cygprofile/symbol_... File tools/cygprofile/symbol_extractor_unittest.py (right): https://codereview.chromium.org/886563002/diff/60001/tools/cygprofile/symbol_... tools/cygprofile/symbol_extractor_unittest.py:25: # This line has an invalid scope On 2015/01/29 15:22:11, pasko wrote: > period? here and below please, if you will Done. https://codereview.chromium.org/886563002/diff/60001/tools/cygprofile/symbol_... tools/cygprofile/symbol_extractor_unittest.py:27: self.assertRaises(AssertionError, symbol_extractor._FromObjdumpLine, line) On 2015/01/29 15:22:11, pasko wrote: > omg this is so cool! I did not know about it, and now absolutely love it. Acknowledged. https://codereview.chromium.org/886563002/diff/60001/tools/cygprofile/symbol_... tools/cygprofile/symbol_extractor_unittest.py:29: line = ('00c1b228 z F .text 00000060 _ZN20trace_event too many') On 2015/01/29 15:22:11, pasko wrote: > this asserts for the same reason as the previous one, so: > s/ z / l / > > same below Done.
The CQ bit was checked by lizeb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/886563002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/737b1473709df32f82adc8bb6654927aad25b7d9 Cr-Commit-Position: refs/heads/master@{#313715} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
