Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
891713002:
Script checking that the observed function order matches an orderfile. (Closed)
DescriptionScript checking that the observed function order matches an orderfile.
BUG=452879
Committed: https://crrev.com/467e5ba225c1e18a7cd9ca292764715c93e4255b
Cr-Commit-Position: refs/heads/master@{#314157}
Patch Set 1 #Patch Set 2 : Spacing, naming, unused imports. #
Total comments: 23
Patch Set 3 : Address comments. #
Total comments: 5
Patch Set 4 : . #Patch Set 5 : . #
Messages
Total messages: 11 (2 generated)
lizeb@chromium.org changed reviewers: + pasko@chromium.org
Hello, Here is another small CL, moving the orderfile check step to a separate script.
Thanks! Having this script it a good idea! Please enjoy my comments in attempt to make things more readable. Having these is _normal_ :) https://codereview.chromium.org/891713002/diff/20001/tools/cygprofile/check_o... File tools/cygprofile/check_orderfile.py (right): https://codereview.chromium.org/891713002/diff/20001/tools/cygprofile/check_o... tools/cygprofile/check_orderfile.py:13: import patch_orderfile nit: alpha https://codereview.chromium.org/891713002/diff/20001/tools/cygprofile/check_o... tools/cygprofile/check_orderfile.py:24: symbol_infos: list of SymbolInfo from the binary nit: s/list/ordered list/ (seems tiny-ish confusing if ordering mentioned in the 1st argument, but omitted on the 2nd) https://codereview.chromium.org/891713002/diff/20001/tools/cygprofile/check_o... tools/cygprofile/check_orderfile.py:27: (misordered_pairs_count, matched_symbols, unmatched_symbols) matched_symbols_count and unmatched_symbols_count? (yay for consistency) https://codereview.chromium.org/891713002/diff/20001/tools/cygprofile/check_o... tools/cygprofile/check_orderfile.py:29: name_to_symbol_info = symbol_extractor.CreateNameToSymbolInfo(symbol_infos) here we are creating a mapping [name -> info], but we need only offsets from the infos. So, with less abstraction, I think the code would be more straightforward to read. Once you create a 'name_to_offset', it's pretty obvious how it's going to be used. You might argue that we need to print 'Unordered pair: info2, info1', but I think (symbol, offset, previous_symbol, previous_offset) would be enough information to be printed. https://codereview.chromium.org/891713002/diff/20001/tools/cygprofile/check_o... tools/cygprofile/check_orderfile.py:42: logging.warning('%d matched symbols, %d un-matched (Only the first %d ' this warning should be under: if missing_count > 0: https://codereview.chromium.org/891713002/diff/20001/tools/cygprofile/check_o... tools/cygprofile/check_orderfile.py:58: for (first, second) in misordered_symbol_infos: Iterating for it second time makes it harder to read: one has to remember how the pairs are laid out. If it were printed as part of the previous loop, it would have been obvious. also, it's a little 'strange' to see (previous_symbol_info == second) And the message could be clearer: The symbol B (offset=X) follows symbol A (offset=Y) in the orderfile. https://codereview.chromium.org/891713002/diff/20001/tools/cygprofile/check_o... tools/cygprofile/check_orderfile.py:66: logging.error('Usage: check_orderfile.py binary orderfile threshold') I'm about polishing again: can threshold be optional with a default==0? It does sound slightly more convenient to _not_ worry about the threshold for triggering an error when you run the script locally to repro a bot issue. https://codereview.chromium.org/891713002/diff/20001/tools/cygprofile/check_o... tools/cygprofile/check_orderfile.py:73: (misordered_pairs_count, _, _) = _CountMisorderedSymbols( Please mention in the comment the non-obvious observation that the symbols not being in the binary is not considered being an error because dead code could have been eliminated at link time. https://codereview.chromium.org/891713002/diff/20001/tools/cygprofile/check_o... File tools/cygprofile/check_orderfile_unittest.py (right): https://codereview.chromium.org/891713002/diff/20001/tools/cygprofile/check_o... tools/cygprofile/check_orderfile_unittest.py:14: symbol_infos = [ using this list as a global constant in the test fixture would prevent repeating ourselves a lot. https://codereview.chromium.org/891713002/diff/20001/tools/cygprofile/check_o... tools/cygprofile/check_orderfile_unittest.py:20: (_, matched_count, _) = check_orderfile._CountMisorderedSymbols( wouldn't it be good to check the other counts here? It's not a lot to add, since can be checked with tuple equality. though if you think that does not add useful checking or makes it significantly less readable, feel free to omit, I don't have a strong opinion on it.
https://codereview.chromium.org/891713002/diff/20001/tools/cygprofile/check_o... File tools/cygprofile/check_orderfile.py (right): https://codereview.chromium.org/891713002/diff/20001/tools/cygprofile/check_o... tools/cygprofile/check_orderfile.py:13: import patch_orderfile On 2015/02/02 10:00:30, pasko wrote: > nit: alpha Done. https://codereview.chromium.org/891713002/diff/20001/tools/cygprofile/check_o... tools/cygprofile/check_orderfile.py:24: symbol_infos: list of SymbolInfo from the binary On 2015/02/02 10:00:30, pasko wrote: > nit: > s/list/ordered list/ > (seems tiny-ish confusing if ordering mentioned in the 1st argument, but omitted > on the 2nd) Done. https://codereview.chromium.org/891713002/diff/20001/tools/cygprofile/check_o... tools/cygprofile/check_orderfile.py:27: (misordered_pairs_count, matched_symbols, unmatched_symbols) On 2015/02/02 10:00:30, pasko wrote: > matched_symbols_count and unmatched_symbols_count? (yay for consistency) Done. https://codereview.chromium.org/891713002/diff/20001/tools/cygprofile/check_o... tools/cygprofile/check_orderfile.py:29: name_to_symbol_info = symbol_extractor.CreateNameToSymbolInfo(symbol_infos) On 2015/02/02 10:00:30, pasko wrote: > here we are creating a mapping [name -> info], but we need only offsets from the > infos. So, with less abstraction, I think the code would be more straightforward > to read. Once you create a 'name_to_offset', it's pretty obvious how it's going > to be used. > > You might argue that we need to print 'Unordered pair: info2, info1', but I > think (symbol, offset, previous_symbol, previous_offset) would be enough > information to be printed. Yes, I think it is better to keep the whole SymbolInfo here, as it is used elsewhere, and also for the time where we'll find something weird (for instance, a symbol being a subset of the offsets of another one, or being 0-sized). But I can change it. wdyt ? https://codereview.chromium.org/891713002/diff/20001/tools/cygprofile/check_o... tools/cygprofile/check_orderfile.py:42: logging.warning('%d matched symbols, %d un-matched (Only the first %d ' On 2015/02/02 10:00:30, pasko wrote: > this warning should be under: > if missing_count > 0: I think that printing the number of matched symbol in all cases is still useful. wdyt ? https://codereview.chromium.org/891713002/diff/20001/tools/cygprofile/check_o... tools/cygprofile/check_orderfile.py:58: for (first, second) in misordered_symbol_infos: On 2015/02/02 10:00:30, pasko wrote: > Iterating for it second time makes it harder to read: one has to remember how > the pairs are laid out. If it were printed as part of the previous loop, it > would have been obvious. > > also, it's a little 'strange' to see (previous_symbol_info == second) > > And the message could be clearer: > The symbol B (offset=X) follows symbol A (offset=Y) in the orderfile. Done, but still printing the whole SymbolInfo (for now). https://codereview.chromium.org/891713002/diff/20001/tools/cygprofile/check_o... tools/cygprofile/check_orderfile.py:66: logging.error('Usage: check_orderfile.py binary orderfile threshold') On 2015/02/02 10:00:30, pasko wrote: > I'm about polishing again: can threshold be optional with a default==0? It does > sound slightly more convenient to _not_ worry about the threshold for triggering > an error when you run the script locally to repro a bot issue. Done. https://codereview.chromium.org/891713002/diff/20001/tools/cygprofile/check_o... tools/cygprofile/check_orderfile.py:73: (misordered_pairs_count, _, _) = _CountMisorderedSymbols( On 2015/02/02 10:00:30, pasko wrote: > Please mention in the comment the non-obvious observation that the symbols not > being in the binary is not considered being an error because dead code could > have been eliminated at link time. Done. https://codereview.chromium.org/891713002/diff/20001/tools/cygprofile/check_o... File tools/cygprofile/check_orderfile_unittest.py (right): https://codereview.chromium.org/891713002/diff/20001/tools/cygprofile/check_o... tools/cygprofile/check_orderfile_unittest.py:14: symbol_infos = [ On 2015/02/02 10:00:30, pasko wrote: > using this list as a global constant in the test fixture would prevent repeating > ourselves a lot. Done. https://codereview.chromium.org/891713002/diff/20001/tools/cygprofile/check_o... tools/cygprofile/check_orderfile_unittest.py:20: (_, matched_count, _) = check_orderfile._CountMisorderedSymbols( On 2015/02/02 10:00:30, pasko wrote: > wouldn't it be good to check the other counts here? It's not a lot to add, since > can be checked with tuple equality. > > though if you think that does not add useful checking or makes it significantly > less readable, feel free to omit, I don't have a strong opinion on it. Done.
one request to s/warning/info/ and one function ordering question otherwise lgtm thanks! https://codereview.chromium.org/891713002/diff/20001/tools/cygprofile/check_o... File tools/cygprofile/check_orderfile.py (right): https://codereview.chromium.org/891713002/diff/20001/tools/cygprofile/check_o... tools/cygprofile/check_orderfile.py:29: name_to_symbol_info = symbol_extractor.CreateNameToSymbolInfo(symbol_infos) On 2015/02/02 13:11:45, lizeb wrote: > On 2015/02/02 10:00:30, pasko wrote: > > here we are creating a mapping [name -> info], but we need only offsets from > the > > infos. So, with less abstraction, I think the code would be more > straightforward > > to read. Once you create a 'name_to_offset', it's pretty obvious how it's > going > > to be used. > > > > You might argue that we need to print 'Unordered pair: info2, info1', but I > > think (symbol, offset, previous_symbol, previous_offset) would be enough > > information to be printed. > > Yes, I think it is better to keep the whole SymbolInfo here, as it is used > elsewhere, and also for the time where we'll find something weird (for instance, > a symbol being a subset of the offsets of another one, or being 0-sized). > > But I can change it. wdyt ? OK, given your consideration, seems like a good enough balance. Thanks! https://codereview.chromium.org/891713002/diff/20001/tools/cygprofile/check_o... tools/cygprofile/check_orderfile.py:42: logging.warning('%d matched symbols, %d un-matched (Only the first %d ' On 2015/02/02 13:11:45, lizeb wrote: > On 2015/02/02 10:00:30, pasko wrote: > > this warning should be under: > > if missing_count > 0: > > I think that printing the number of matched symbol in all cases is still useful. > wdyt ? ok, then let's make it log.info to avoid the sea of red herrings in the output from the bots. https://codereview.chromium.org/891713002/diff/20001/tools/cygprofile/check_o... tools/cygprofile/check_orderfile.py:58: for (first, second) in misordered_symbol_infos: On 2015/02/02 13:11:45, lizeb wrote: > On 2015/02/02 10:00:30, pasko wrote: > > Iterating for it second time makes it harder to read: one has to remember how > > the pairs are laid out. If it were printed as part of the previous loop, it > > would have been obvious. > > > > also, it's a little 'strange' to see (previous_symbol_info == second) > > > > And the message could be clearer: > > The symbol B (offset=X) follows symbol A (offset=Y) in the orderfile. > > Done, but still printing the whole SymbolInfo (for now). Acknowledged. https://codereview.chromium.org/891713002/diff/40001/tools/cygprofile/patch_o... File tools/cygprofile/patch_orderfile.py (right): https://codereview.chromium.org/891713002/diff/40001/tools/cygprofile/patch_o... tools/cygprofile/patch_orderfile.py:20: 1. Get the symbol infos (offset, length, name) from the binary nit: (name, offset, size, section) https://codereview.chromium.org/891713002/diff/40001/tools/cygprofile/patch_o... tools/cygprofile/patch_orderfile.py:125: def GetSymbolsFromOrderfile(filename): thanks for this change. So this natirally leads me to ask for more :) What order of functions should be used? Are there any python/google/whatever guidelines? Maybe exported functions being on top/bottom? In Chromium C++ we have a guideline of having the functions defined in the .cc file in the _same_ order as declared in the header. I don't particularly like it, but it leads to less surprises for _some_ devs.
Thank you ! https://codereview.chromium.org/891713002/diff/40001/tools/cygprofile/patch_o... File tools/cygprofile/patch_orderfile.py (right): https://codereview.chromium.org/891713002/diff/40001/tools/cygprofile/patch_o... tools/cygprofile/patch_orderfile.py:20: 1. Get the symbol infos (offset, length, name) from the binary On 2015/02/02 14:32:37, pasko wrote: > nit: > (name, offset, size, section) Done. https://codereview.chromium.org/891713002/diff/40001/tools/cygprofile/patch_o... tools/cygprofile/patch_orderfile.py:125: def GetSymbolsFromOrderfile(filename): On 2015/02/02 14:32:37, pasko wrote: > thanks for this change. So this natirally leads me to ask for more :) > > What order of functions should be used? Are there any python/google/whatever > guidelines? Maybe exported functions being on top/bottom? > > In Chromium C++ we have a guideline of having the functions defined in the .cc > file in the _same_ order as declared in the header. I don't particularly like > it, but it leads to less surprises for _some_ devs. I would argue that once a function has been written, it should not move within its file for no reason, to keep the noise in "git blame" output low. So I would leave this one alone for now.
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/891713002/80001
https://codereview.chromium.org/891713002/diff/40001/tools/cygprofile/patch_o... File tools/cygprofile/patch_orderfile.py (right): https://codereview.chromium.org/891713002/diff/40001/tools/cygprofile/patch_o... tools/cygprofile/patch_orderfile.py:125: def GetSymbolsFromOrderfile(filename): On 2015/02/02 15:37:05, lizeb wrote: > On 2015/02/02 14:32:37, pasko wrote: > > thanks for this change. So this natirally leads me to ask for more :) > > > > What order of functions should be used? Are there any python/google/whatever > > guidelines? Maybe exported functions being on top/bottom? > > > > In Chromium C++ we have a guideline of having the functions defined in the .cc > > file in the _same_ order as declared in the header. I don't particularly like > > it, but it leads to less surprises for _some_ devs. > > I would argue that once a function has been written, it should not move within > its file for no reason, to keep the noise in "git blame" output low. So I would > leave this one alone for now. git blame -C ? but yeah, I don't like moving functions around unnecessarily either. Here it's a bit of a special case, where refactoring moved a lot of things anyway, and, as we know, the primary purpose of refactoring is how the final version of the code looks, so .. spoiled git blame is really not an issue.
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/467e5ba225c1e18a7cd9ca292764715c93e4255b Cr-Commit-Position: refs/heads/master@{#314157} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
