Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
784333002:
Check that all symbolized methods in the output orderfile are in their own linker section in the or… (Closed)
DescriptionCheck that all symbolized methods in the output orderfile are in their own linker section in the original object files.
BUG=440018
Patch Set 1 #
Total comments: 24
Patch Set 2 : Address code review comments. #
Total comments: 10
Patch Set 3 : Fix nits. #
Total comments: 6
Patch Set 4 : Refine newline handling #Messages
Total messages: 14 (3 generated)
azarchs@chromium.org changed reviewers: + pasko@chromium.org
This should turn the orderfile bot green. Underlying issues still need to be fixed.
Thank you. It's really important to have the bot green, and extra thanks for the warnings to help triage problems on the bot. https://codereview.chromium.org/784333002/diff/1/tools/cygprofile/symbolize.py File tools/cygprofile/symbolize.py (right): https://codereview.chromium.org/784333002/diff/1/tools/cygprofile/symbolize.p... tools/cygprofile/symbolize.py:66: def GetOutputLines(cmd): we are ignoring stderr (which is arguable also an output?), so probably a good idea to call it like: GetStdoutFromCommand() https://codereview.chromium.org/784333002/diff/1/tools/cygprofile/symbolize.p... tools/cygprofile/symbolize.py:178: while index < len(output): with the given arguments addr2line always outputs 2 lines, so it would be *much* clearer to: assert(len(output) == 2) line = ':'.join(output) wtf_per_day += 1 https://codereview.chromium.org/784333002/diff/1/tools/cygprofile/symbolize.p... tools/cygprofile/symbolize.py:183: def ObjectFiles(obj_dir): GetObjectFiles https://codereview.chromium.org/784333002/diff/1/tools/cygprofile/symbolize.p... tools/cygprofile/symbolize.py:184: """ Gets the list of object files in the output folder """ this is on linux, so s/folder/directory./ :) https://codereview.chromium.org/784333002/diff/1/tools/cygprofile/symbolize.p... tools/cygprofile/symbolize.py:194: of the object files """ s/files/files./ https://codereview.chromium.org/784333002/diff/1/tools/cygprofile/symbolize.p... tools/cygprofile/symbolize.py:199: objects = GetOutputLines(cmd) in lunix/binutils land 'object' and 'object file' are synonymous, and these are rather symbol_lines (which we then later extract a symbols from) https://codereview.chromium.org/784333002/diff/1/tools/cygprofile/symbolize.p... tools/cygprofile/symbolize.py:201: items = object_line.split() it is non-obvious that simple splitting like this works because the one-character fields are not necessarily separated by spaces (unfortunately). But we can do it because in object files we see only these patterns: " g F" <- global function " l F" <- local function " w F" <- weak function (overrideable at link-time, both dynamic and static linking) all of them can be parsed as 2 fields. Please explain that in a comment. https://codereview.chromium.org/784333002/diff/1/tools/cygprofile/symbolize.p... tools/cygprofile/symbolize.py:204: symbol = items[len(items) - 1] more pythonic, but arguably less readable: symbol = items[-1] what is our style? https://codereview.chromium.org/784333002/diff/1/tools/cygprofile/symbolize.p... tools/cygprofile/symbolize.py:210: ' in conflicting sections ' + section + I really don't see a problem with having a single symbol be in different sections. It may mean different valid things (like not supposed to be linked together, or having a weak symbol and a strong one in a different file). https://codereview.chromium.org/784333002/diff/1/tools/cygprofile/symbolize.p... tools/cygprofile/symbolize.py:213: sys.stderr.write('WARNING: Symbol ' + symbol + should we limit the number of messages like this to say a 100? I am afraid that something may go completely wrong, and we would print 250000 warningz https://codereview.chromium.org/784333002/diff/1/tools/cygprofile/symbolize.p... tools/cygprofile/symbolize.py:216: symbol_map[symbol] = section section_map or symbol_to_section_map would be better because when you write: """ section_map[symbol] """ it more clearly suggests that we are mapping symbols to sections. https://codereview.chromium.org/784333002/diff/1/tools/cygprofile/symbolize.p... tools/cygprofile/symbolize.py:273: sys.stderr.write('WARNING: Unknown symbol ' + symbol + '\n') so this happens when a symbol is in the final library, but we did not find it in an object, right? It makes sense to explain it in the comment or in the message itself. Same here about the amount of warnings printed, can we limit with a 100? Maybe a class to print 100 first messages and ignore the rest? I see a pattern here :)
Thank you. It's really important to have the bot green, and extra thanks for the warnings to help triage problems on the bot.
Can't test this until I figure out why the build is broken. https://codereview.chromium.org/784333002/diff/1/tools/cygprofile/symbolize.py File tools/cygprofile/symbolize.py (right): https://codereview.chromium.org/784333002/diff/1/tools/cygprofile/symbolize.p... tools/cygprofile/symbolize.py:66: def GetOutputLines(cmd): On 2014/12/10 12:07:05, pasko wrote: > we are ignoring stderr (which is arguable also an output?), so probably a good > idea to call it like: > GetStdoutFromCommand() Done. https://codereview.chromium.org/784333002/diff/1/tools/cygprofile/symbolize.p... tools/cygprofile/symbolize.py:178: while index < len(output): On 2014/12/10 12:07:05, pasko wrote: > with the given arguments addr2line always outputs 2 lines, so it would be *much* > clearer to: > assert(len(output) == 2) > line = ':'.join(output) > > wtf_per_day += 1 Done. https://codereview.chromium.org/784333002/diff/1/tools/cygprofile/symbolize.p... tools/cygprofile/symbolize.py:183: def ObjectFiles(obj_dir): On 2014/12/10 12:07:05, pasko wrote: > GetObjectFiles Done. https://codereview.chromium.org/784333002/diff/1/tools/cygprofile/symbolize.p... tools/cygprofile/symbolize.py:184: """ Gets the list of object files in the output folder """ On 2014/12/10 12:07:05, pasko wrote: > this is on linux, so s/folder/directory./ :) Done. https://codereview.chromium.org/784333002/diff/1/tools/cygprofile/symbolize.p... tools/cygprofile/symbolize.py:199: objects = GetOutputLines(cmd) On 2014/12/10 12:07:05, pasko wrote: > in lunix/binutils land 'object' and 'object file' are synonymous, and these are > rather symbol_lines (which we then later extract a symbols from) Done. https://codereview.chromium.org/784333002/diff/1/tools/cygprofile/symbolize.p... tools/cygprofile/symbolize.py:201: items = object_line.split() On 2014/12/10 12:07:05, pasko wrote: > it is non-obvious that simple splitting like this works because the > one-character fields are not necessarily separated by spaces (unfortunately). > But we can do it because in object files we see only these patterns: > " g F" <- global function > " l F" <- local function > " w F" <- weak function (overrideable at link-time, both dynamic and static > linking) > > all of them can be parsed as 2 fields. Please explain that in a comment. Done. https://codereview.chromium.org/784333002/diff/1/tools/cygprofile/symbolize.p... tools/cygprofile/symbolize.py:204: symbol = items[len(items) - 1] On 2014/12/10 12:07:05, pasko wrote: > more pythonic, but arguably less readable: > symbol = items[-1] > > what is our style? Style guide says nothing on the subject. I prefer the more readable version. https://codereview.chromium.org/784333002/diff/1/tools/cygprofile/symbolize.p... tools/cygprofile/symbolize.py:210: ' in conflicting sections ' + section + On 2014/12/10 12:07:05, pasko wrote: > I really don't see a problem with having a single symbol be in different > sections. It may mean different valid things (like not supposed to be linked > together, or having a weak symbol and a strong one in a different file). It doesn't currently happen. If it did, which section would be specify in the order file? All of them? https://codereview.chromium.org/784333002/diff/1/tools/cygprofile/symbolize.p... tools/cygprofile/symbolize.py:213: sys.stderr.write('WARNING: Symbol ' + symbol + On 2014/12/10 12:07:04, pasko wrote: > should we limit the number of messages like this to say a 100? I am afraid that > something may go completely wrong, and we would print 250000 warningz Done. https://codereview.chromium.org/784333002/diff/1/tools/cygprofile/symbolize.p... tools/cygprofile/symbolize.py:216: symbol_map[symbol] = section On 2014/12/10 12:07:05, pasko wrote: > section_map or symbol_to_section_map would be better because when you write: > """ > section_map[symbol] > """ > > it more clearly suggests that we are mapping symbols to sections. Done. https://codereview.chromium.org/784333002/diff/1/tools/cygprofile/symbolize.p... tools/cygprofile/symbolize.py:273: sys.stderr.write('WARNING: Unknown symbol ' + symbol + '\n') On 2014/12/10 12:07:05, pasko wrote: > so this happens when a symbol is in the final library, but we did not find it in > an object, right? It makes sense to explain it in the comment or in the message > itself. > > Same here about the amount of warnings printed, can we limit with a 100? Maybe a > class to print 100 first messages and ignore the rest? I see a pattern here :) Done.
LGTM with a few cosmetic changes https://codereview.chromium.org/784333002/diff/1/tools/cygprofile/symbolize.py File tools/cygprofile/symbolize.py (right): https://codereview.chromium.org/784333002/diff/1/tools/cygprofile/symbolize.p... tools/cygprofile/symbolize.py:204: symbol = items[len(items) - 1] On 2014/12/10 17:13:33, azarchs wrote: > On 2014/12/10 12:07:05, pasko wrote: > > more pythonic, but arguably less readable: > > symbol = items[-1] > > > > what is our style? > > Style guide says nothing on the subject. I prefer the more readable version. Acknowledged. https://codereview.chromium.org/784333002/diff/20001/tools/cygprofile/symboli... File tools/cygprofile/symbolize.py (right): https://codereview.chromium.org/784333002/diff/20001/tools/cygprofile/symboli... tools/cygprofile/symbolize.py:193: def write(self, message): functions should start from a capital letter https://codereview.chromium.org/784333002/diff/20001/tools/cygprofile/symboli... tools/cygprofile/symbolize.py:201: ' more warnings for ' + message) I guess Write(str(X) + 'Y' + Z) is for consistency within this file :) otherwise looks alien to python, where people write: Write('%d Y %s' % (X, Z)) OK for now, since it's not a good time to fix other occurrences. https://codereview.chromium.org/784333002/diff/20001/tools/cygprofile/symboli... tools/cygprofile/symbolize.py:208: stderr = WarningCollector(300) having it named stderr is confusing, since it is not really sys.stderr how about "warnings"? it would be like: warnings.Write(message) https://codereview.chromium.org/784333002/diff/20001/tools/cygprofile/symboli... tools/cygprofile/symbolize.py:276: symbol_map = SymbolToSection(obj_dir) this is also a symbol_to_section_map or a section_map https://codereview.chromium.org/784333002/diff/20001/tools/cygprofile/symboli... tools/cygprofile/symbolize.py:316: symbol_not_found_warnings.WriteEnd('did not find function') So it would say: 1. X more warnings for no known section for symbol 2. X more warnings for did not find function this looks slightly better to me: 1. X more warnings for: no known section for symbol 2. X more warnings for: did not find function
https://codereview.chromium.org/784333002/diff/20001/tools/cygprofile/symboli... File tools/cygprofile/symbolize.py (right): https://codereview.chromium.org/784333002/diff/20001/tools/cygprofile/symboli... tools/cygprofile/symbolize.py:193: def write(self, message): On 2014/12/10 17:56:09, pasko wrote: > functions should start from a capital letter Done. https://codereview.chromium.org/784333002/diff/20001/tools/cygprofile/symboli... tools/cygprofile/symbolize.py:201: ' more warnings for ' + message) On 2014/12/10 17:56:09, pasko wrote: > I guess Write(str(X) + 'Y' + Z) is for consistency within this file :) > > otherwise looks alien to python, where people write: > Write('%d Y %s' % (X, Z)) > > OK for now, since it's not a good time to fix other occurrences. Acknowledged. https://codereview.chromium.org/784333002/diff/20001/tools/cygprofile/symboli... tools/cygprofile/symbolize.py:208: stderr = WarningCollector(300) On 2014/12/10 17:56:09, pasko wrote: > having it named stderr is confusing, since it is not really sys.stderr > > how about "warnings"? > > it would be like: > warnings.Write(message) Done. https://codereview.chromium.org/784333002/diff/20001/tools/cygprofile/symboli... tools/cygprofile/symbolize.py:276: symbol_map = SymbolToSection(obj_dir) On 2014/12/10 17:56:09, pasko wrote: > this is also a symbol_to_section_map or a section_map Done. https://codereview.chromium.org/784333002/diff/20001/tools/cygprofile/symboli... tools/cygprofile/symbolize.py:316: symbol_not_found_warnings.WriteEnd('did not find function') On 2014/12/10 17:56:09, pasko wrote: > So it would say: > 1. X more warnings for no known section for symbol > 2. X more warnings for did not find function > > this looks slightly better to me: > 1. X more warnings for: no known section for symbol > 2. X more warnings for: did not find function Done.
https://codereview.chromium.org/784333002/diff/40001/tools/cygprofile/symboli... File tools/cygprofile/symbolize.py (right): https://codereview.chromium.org/784333002/diff/40001/tools/cygprofile/symboli... tools/cygprofile/symbolize.py:201: ' more warnings for: ' + message) does not have a '\n' https://codereview.chromium.org/784333002/diff/40001/tools/cygprofile/symboli... tools/cygprofile/symbolize.py:227: ' and ' + symbol_to_section_map[symbol] + '\n') please move the '\n' to WarningCollector.Write, less repetitions https://codereview.chromium.org/784333002/diff/40001/tools/cygprofile/symboli... tools/cygprofile/symbolize.py:293: unknown_symbol_warnings.write( s/write/Write/ here and below
I think this is ready to go. I've tested it. https://codereview.chromium.org/784333002/diff/40001/tools/cygprofile/symboli... File tools/cygprofile/symbolize.py (right): https://codereview.chromium.org/784333002/diff/40001/tools/cygprofile/symboli... tools/cygprofile/symbolize.py:201: ' more warnings for: ' + message) On 2014/12/11 10:25:30, pasko wrote: > does not have a '\n' Done. https://codereview.chromium.org/784333002/diff/40001/tools/cygprofile/symboli... tools/cygprofile/symbolize.py:227: ' and ' + symbol_to_section_map[symbol] + '\n') On 2014/12/11 10:25:30, pasko wrote: > please move the '\n' to WarningCollector.Write, less repetitions Done. https://codereview.chromium.org/784333002/diff/40001/tools/cygprofile/symboli... tools/cygprofile/symbolize.py:293: unknown_symbol_warnings.write( On 2014/12/11 10:25:30, pasko wrote: > s/write/Write/ here and below Done.
The CQ bit was checked by azarchs@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/784333002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
PTAL |
