Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
339853004:
binary_size_tool: fix for ambiguous addr2line output (Closed)
|
Created:
6 years, 6 months ago by sebastianl Modified:
6 years, 5 months ago CC:
chromium-reviews, klundberg+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy-cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src@master Project:
chromium Visibility:
Public. |
Descriptionbinary_size_tool: fix for ambiguous addr2line output
Sometimes the output of addr2line is ambiguous, example:
unicode.cc:0
and does not include the absolute path of the source file. This fix
is mostly a port of andrewhaydens solution to disambiguate the path.
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=280303
Patch Set 1 #
Total comments: 6
Patch Set 2 : update according to andrewhaydens recommendations #Patch Set 3 : fixed logical error in counters #Patch Set 4 : rewrote check for ambiguous paths (now works if current working directory is not the source directo… #
Total comments: 13
Patch Set 5 : update according to primiano and andrewhaydens instructions #
Total comments: 13
Patch Set 6 : renamed disambiguation status booleans, rewrote some logic #
Total comments: 11
Patch Set 7 : changed descriptions, lookup_table => disambiguation_table rename, can now use relative paths witho… #
Total comments: 8
Patch Set 8 : updated descriptions, rebased, s/os.path.isabs/posixpath.isabs/ #
Messages
Total messages: 38 (0 generated)
Ported andrewhaydens disambiguation solution to addr2line when addr2line returns an ambigous source path. Does it look okay to you?
https://codereview.chromium.org/339853004/diff/1/build/android/pylib/symbols/... File build/android/pylib/symbols/elf_symbolizer.py (right): https://codereview.chromium.org/339853004/diff/1/build/android/pylib/symbols/... build/android/pylib/symbols/elf_symbolizer.py:96: Disambiguation means to resolve ambigious source_paths, ambigious -> ambiguous (Spelling) You may also want to add a short note something like: In some toolchains only the name of the source file is output, without any path information; disambiguation searches through the source directory specified by --disambiguate_source_path for files whose name matches. If there are multiple files with the same name, disambiguation will fail. https://codereview.chromium.org/339853004/diff/1/build/android/pylib/symbols/... build/android/pylib/symbols/elf_symbolizer.py:97: example turn addr2line output "unicode.cc" into a full and absolute "for example" instead of "example" https://codereview.chromium.org/339853004/diff/1/build/android/pylib/symbols/... build/android/pylib/symbols/elf_symbolizer.py:99: disambiguate_source_path: The path of the source code that the I'd say: the path to the directory where source files are located, used for disambiguating paths. https://codereview.chromium.org/339853004/diff/1/build/android/pylib/symbols/... build/android/pylib/symbols/elf_symbolizer.py:364: # Strip the common prefix to determine whether the source is ambigous "ambiguous" https://codereview.chromium.org/339853004/diff/1/build/android/pylib/symbols/... build/android/pylib/symbols/elf_symbolizer.py:373: if not path.find('/') != -1: Double negation is hard to reason about. How about: if not path.find('/'): https://codereview.chromium.org/339853004/diff/1/tools/binary_size/run_binary... File tools/binary_size/run_binary_size_analysis.py (right): https://codereview.chromium.org/339853004/diff/1/tools/binary_size/run_binary... tools/binary_size/run_binary_size_analysis.py:485: self.disambiguations = 0 In the old code I also counted the number of disambiguation attempts that failed, and output that number as well. This is useful because it gives you an idea of whether or not disambiguation is working and how effectively. This is still an improvement so I'm not saying you have to do this right now, but it would be nice to keep the feature.
On 2014/06/17 11:36:27, Andrew Hayden wrote: > https://codereview.chromium.org/339853004/diff/1/build/android/pylib/symbols/... > File build/android/pylib/symbols/elf_symbolizer.py (right): > > https://codereview.chromium.org/339853004/diff/1/build/android/pylib/symbols/... > build/android/pylib/symbols/elf_symbolizer.py:96: Disambiguation means to > resolve ambigious source_paths, > ambigious -> ambiguous (Spelling) > > You may also want to add a short note something like: > In some toolchains only the name of the source file is output, without any path > information; disambiguation searches through the source directory specified by > --disambiguate_source_path for files whose name matches. If there are multiple > files with the same name, disambiguation will fail. > > https://codereview.chromium.org/339853004/diff/1/build/android/pylib/symbols/... > build/android/pylib/symbols/elf_symbolizer.py:97: example turn addr2line output > "unicode.cc" into a full and absolute > "for example" instead of "example" > > https://codereview.chromium.org/339853004/diff/1/build/android/pylib/symbols/... > build/android/pylib/symbols/elf_symbolizer.py:99: disambiguate_source_path: The > path of the source code that the > I'd say: the path to the directory where source files are located, used for > disambiguating paths. > > https://codereview.chromium.org/339853004/diff/1/build/android/pylib/symbols/... > build/android/pylib/symbols/elf_symbolizer.py:364: # Strip the common prefix to > determine whether the source is ambigous > "ambiguous" > > https://codereview.chromium.org/339853004/diff/1/build/android/pylib/symbols/... > build/android/pylib/symbols/elf_symbolizer.py:373: if not path.find('/') != -1: > Double negation is hard to reason about. How about: > if not path.find('/'): > > https://codereview.chromium.org/339853004/diff/1/tools/binary_size/run_binary... > File tools/binary_size/run_binary_size_analysis.py (right): > > https://codereview.chromium.org/339853004/diff/1/tools/binary_size/run_binary... > tools/binary_size/run_binary_size_analysis.py:485: self.disambiguations = 0 > In the old code I also counted the number of disambiguation attempts that > failed, and output that number as well. This is useful because it gives you an > idea of whether or not disambiguation is working and how effectively. This is > still an improvement so I'm not saying you have to do this right now, but it > would be nice to keep the feature. Fixed the typos and the double negation in my latest upload. I did also change the way the callback handler spits out information, and added a return-carrier character at the beginning to prevent flooding of the terminal.
The CQ bit was checked by sebastianl@opera.com
The CQ bit was unchecked by sebastianl@opera.com
Fixed a new version now. Please have a look and share your thoughts
Minor bits only, aside from that this looks pretty good. One last patchset? https://codereview.chromium.org/339853004/diff/60001/build/android/pylib/symb... File build/android/pylib/symbols/elf_symbolizer.py (right): https://codereview.chromium.org/339853004/diff/60001/build/android/pylib/symb... build/android/pylib/symbols/elf_symbolizer.py:369: if not source_path is None and not source_path.startswith('/'): Again, let's avoid double-negation: if source_path and not source_path.startswith('/') This is clearer than if not source_path is None... https://codereview.chromium.org/339853004/diff/60001/build/android/pylib/symb... build/android/pylib/symbols/elf_symbolizer.py:372: disambiguated = not failed_disambiguation As written, you don't need two booleans. They're just being assigned inverse values, so diambiguated implies !failed_disambiguated and failed_disambiguated implies !disambiguated. I think what you want here is more like: was_ambiguous: True iff the path was a bare file, no path information. disambiguated: True iff was_ambiguous == True AND we were able to successfully disambiguate the path. Then you have this truth table (was_ambiguous,dismabiguated): (false, false): full path info produced by nm (false, true): full path info produced by nm (true, false): raw filename produced by nm, failed to disambiguate (true, true): raw filename produced by nm, succeeded in disambiguation attempt Edit: OK, you could also use failed_disambiguation (like in later code), and flip the meaning in the truth table above. https://codereview.chromium.org/339853004/diff/60001/build/android/pylib/symb... build/android/pylib/symbols/elf_symbolizer.py:375: if not source_path is None: Again let's avoid double negation: if source_path or if you really prefer I think python allows: if source_path is not None
Besides the comments I wrote inline, I have a general doubt on elf_symbolizer. Currently, in the majority of cases, elf_symbolizer produces paths like /home/of/some/random/guy/file.cc. Now, it is my understanding that the proposed change makes it so that bare file names are relativized w.r.t of the passed root path. I still have the doubt of what happens to absolute paths which do not have any sense on the local machine (like /home/of/some/random/guy/file.cc, which refers to the machine of whoever compiled the original code?) Shouldn't you need to generalize this logic to rebase also absolute file paths using the disambiguation logic? I.e. something that remaps /home/of/some/random/guy/file.cc to /home/sebastiani/file.cc Unless I'm missing something, this change seems a way in the middle, in the sense that symbols with bare file names will get the right absolute path, but symbols with a full path will point to an arcane full path which doesn't exists int he current machine. https://codereview.chromium.org/339853004/diff/60001/build/android/pylib/symb... File build/android/pylib/symbols/elf_symbolizer.py (right): https://codereview.chromium.org/339853004/diff/60001/build/android/pylib/symb... build/android/pylib/symbols/elf_symbolizer.py:16: from sets import Set just use the builtin set. We don't target python < 2.6 https://codereview.chromium.org/339853004/diff/60001/build/android/pylib/symb... build/android/pylib/symbols/elf_symbolizer.py:95: disambiguate: Whether to run a disambiguation process or not. Can we use just one variable (call it source_root_path), which is None by default, and when set kicks in the disambiguation logic? We definitely don't need both (is there any sense by setting disambiguate=True and disambiguation_source_path = '' ?) https://codereview.chromium.org/339853004/diff/60001/build/android/pylib/symb... build/android/pylib/symbols/elf_symbolizer.py:98: path. In some toolchains only the name of the source file is output, I'd love to know more about this btw. In which cases do you get symbols which have only a file name and not the path? This sounds like broken compiler / compiler flags. https://codereview.chromium.org/339853004/diff/60001/build/android/pylib/symb... build/android/pylib/symbols/elf_symbolizer.py:182: def _CreateDisambiguationTable(self, src_root_path): I don't really like this approach. This is trying to index all source files in the root path. In the case of large code bases like chrome/android this feels overkilling. (Also this pretends to know the extensions which are relevant for the caller, which looks terrible). At very last, can we do the other way round? That is: keep only a single hashtable 'file.c' -> '/path/file.c', and whenever you encounter a bare filename with no path which is not present in the hashtable, you walk the oath looking for the file and populate the table (OS FS cache will be your friend). I'd blow away this entire table. https://codereview.chromium.org/339853004/diff/60001/build/android/pylib/symb... build/android/pylib/symbols/elf_symbolizer.py:184: Disambiguation: This comment is redundant. You already explained the concept in __init__ https://codereview.chromium.org/339853004/diff/60001/build/android/pylib/symb... build/android/pylib/symbols/elf_symbolizer.py:376: source_path = os.path.abspath(source_path) What is this for? Looks like that even if you're not disambiguating, you force any relative path to be translated using cwd as base? This looks a very arguable assumption. https://codereview.chromium.org/339853004/diff/60001/build/android/pylib/symb... build/android/pylib/symbols/elf_symbolizer.py:460: def __init__(self, name, source_path, source_line, disambiguated=False, Do you need to pass these booleans at all? Can't you infer that in the caller by looking at the presence of source_path?
https://codereview.chromium.org/339853004/diff/60001/build/android/pylib/symb... File build/android/pylib/symbols/elf_symbolizer.py (right): https://codereview.chromium.org/339853004/diff/60001/build/android/pylib/symb... build/android/pylib/symbols/elf_symbolizer.py:79: disambiguate=False, disambiguation_source_path=''): I had a quick chat with Andrew and we feel the best thing to do inside here is pass two arguments: 1. source_root_path (optional): the path to the source tree on the local disk (for disambiguation) 2. strip_base_path: the base path to strip off the symbol information (to address the problem mentioned in my earlier comment). In essence the behavior should be the following: in presence of both these variables, elf_symbolizer will strive to produce paths which are absolute and refer to existing stuff in the local filesystem. Here is en example: src_root_path = /home/primiano/chrome strip_base_path = /foo/chrome elf_symbolizer will translate the paths as follows: bar.c -> /home/primiano/chrome/somewhere/bar.c if there was one and only one hit for bar.c in source_root_path baz.c -> baz.c if we couldn't disambiguate any (or found than one hits). /foo/chrome/bar.c -> /home/primiano/chrome/bar.c (strip off strip_base_path and prepend source_root_path) https://codereview.chromium.org/339853004/diff/60001/build/android/pylib/symb... build/android/pylib/symbols/elf_symbolizer.py:182: def _CreateDisambiguationTable(self, src_root_path): After talking with Andrew, given your use case, I'm ok with indexing the paths here. Just we have to simplify this code. I don't like caring at all about the extensions. Indexing everything will not be that worse that indexing the c/C++ files, and will save a lot of complexity. In other words, this thing should be as easy and legible as the following four lines of python: src_paths_index = {} for dpath, _, fnames in os.walk(src_root_path): for fname in fnames: src_paths_index[fname] = os.path.join(dpath, fname) if fname not in src_paths_index else None This will produce a table which keys are file names and entries are either a full path (if unique) or None (if ambiguous) https://codereview.chromium.org/339853004/diff/60001/build/android/pylib/symb... build/android/pylib/symbols/elf_symbolizer.py:376: source_path = os.path.abspath(source_path) At this point you also check if source_path.startswith(strip_base_path), and if so replace it with the src_root_path
On 2014/06/18 09:49:11, Primiano Tucci wrote:
> Besides the comments I wrote inline, I have a general doubt on elf_symbolizer.
> Currently, in the majority of cases, elf_symbolizer produces paths like
> /home/of/some/random/guy/file.cc.
> Now, it is my understanding that the proposed change makes it so that bare
file
> names are relativized w.r.t of the passed root path.
> I still have the doubt of what happens to absolute paths which do not have any
> sense on the local machine (like /home/of/some/random/guy/file.cc, which
refers
> to the machine of whoever compiled the original code?)
>
> Shouldn't you need to generalize this logic to rebase also absolute file paths
> using the disambiguation logic?
> I.e. something that remaps /home/of/some/random/guy/file.cc to
> /home/sebastiani/file.cc
>
> Unless I'm missing something, this change seems a way in the middle, in the
> sense that symbols with bare file names will get the right absolute path, but
> symbols with a full path will point to an arcane full path which doesn't
exists
> int he current machine.
>
The reason I wrote this fix was because I ran the binary analyzing tool on
libcontent_shell_content_view.so, and the results came back all weird. Some
paths were absolute, and others not. This was tested on about five different
computers; I assume that version of toolchains varied due to the variation of
Linux distrobutions. I also got the same result using binutils 2.22 as 2.24.
The problem was that some symbols resided in the '/' path, and some wasn't; even
if they were defined in the same file. What caused this turned out to be that
addr2line sometimes returned an ambiguous path (just a file name). This fix is
the solution to that particular problem.
We can however trim the beginning och each path to remove a common prefix in
order to make the results general and not varied depending on where the source
of chromium is located.
Example of a symbol where addr2line returns an ambiguous path:
0x2a9e2f5 in libcontent_shell_content_view.so compiled yesterday using debug
symbols (ninja -C out/Debug content_shell_apk)
cmd: "addr2line 2a9e2f5 -C -f -e
./out/Debug/lib/libcontent_shell_content_view.so"
result: "$d
SMILTime.cpp:?"
It does indeed seem that this is some kind of broken compilation flag.
I just saw your latest message. I'll patch it up :)
Just so that you guys are aware, I have been in touch with the "gold" folks (the linker), and they seem cool with us attempting to patch up gold to produce a little more information. Torne@ suggested many months ago that the "true" solution to this lies in having the linker tell us precisely where it gets every symbol from, and this is probably true. Eventually, in the mystical free time that we all have, we will get around to making the linker give us an accurate map, and then hopefully we can retire all of these "elegant workarounds" (.... :P) and just get the info we need, every time. So, one day this will all be a happy memory.
I updated according to your instructions. Did I get it right, or did I misinterpret?
Close but there are some unresolved Andrew's comments. As a general principle, keep the code concise as long as it is readable. It's one of the nice virtues of python, expressivity and clarity in few lines. https://codereview.chromium.org/339853004/diff/80001/build/android/pylib/symb... File build/android/pylib/symbols/elf_symbolizer.py (left): https://codereview.chromium.org/339853004/diff/80001/build/android/pylib/symb... build/android/pylib/symbols/elf_symbolizer.py:17: Nit: Keep the extra blankline here. Code styles provides 2 \n between top levels IIRC https://codereview.chromium.org/339853004/diff/80001/build/android/pylib/symb... File build/android/pylib/symbols/elf_symbolizer.py (right): https://codereview.chromium.org/339853004/diff/80001/build/android/pylib/symb... build/android/pylib/symbols/elf_symbolizer.py:94: disambiguate: Whether to run a disambiguation process or not. Please update the doc here with the renamed variables. https://codereview.chromium.org/339853004/diff/80001/build/android/pylib/symb... build/android/pylib/symbols/elf_symbolizer.py:118: self.disambiguate = source_root_path is not None You don't seem to make any use of self.disambiguate. Can you just directly use source_root_path in the check. Also, typically it's nicer to be concise in python, unless there is a functional need for it (if foo vs if foo is not None). In essence, remove 118, and turn line 122 into if source_root_path: https://codereview.chromium.org/339853004/diff/80001/build/android/pylib/symb... build/android/pylib/symbols/elf_symbolizer.py:119: self.lookup_table = {} nit: _lookup_table (add _ prefix) as this is a private detail. https://codereview.chromium.org/339853004/diff/80001/build/android/pylib/symb... build/android/pylib/symbols/elf_symbolizer.py:123: self._CreateDisambiguationTable(source_root_path) Ehm, if you store source_root_path as a field into self, passing that as an argument of _CreateDisambiguationTable feels redundant. Just use self.source_root_path in _CreateDisambiguationTable, no? https://codereview.chromium.org/339853004/diff/80001/build/android/pylib/symb... build/android/pylib/symbols/elf_symbolizer.py:182: def _CreateDisambiguationTable(self, src_root_path): Is there a reason why you couldn't use those 4 lines of code I pasted in the previous comment? This seems a bit redundant and unnecessarily verbose: you populate the table, keep track of duplicates, and then remove the duplicates from the table https://codereview.chromium.org/339853004/diff/80001/build/android/pylib/symb... build/android/pylib/symbols/elf_symbolizer.py:183: """ Creates a table of files used for disambiguation later I think this comment is pleonastic. It tells that _CreateDisambiguationTable creates a table of files used for disambiguation. The name is explicative enough itself. Also, doesn't matter if you write that this piece of code is andrewhayden's. If something goes wrong people will git blame and come back to you anyways, not Andrew :) In essence, remove this """ comment here. It makes the file longer without providing any useful information. https://codereview.chromium.org/339853004/diff/80001/build/android/pylib/symb... build/android/pylib/symbols/elf_symbolizer.py:354: disambiguated = False I think that Andrew suggested that the two bools are unnecessary? https://codereview.chromium.org/339853004/diff/80001/build/android/pylib/symb... build/android/pylib/symbols/elf_symbolizer.py:357: if source_path and not source_path.startswith('/'): I think this line is completely unnecessary. Just look if you have the entry in the table. If you have it in the table, the path must have been necessarily != None and a relative path. I.s., this should just be source_path = self._symbolizer.lookup_table.get(source_path) or source_path https://codereview.chromium.org/339853004/diff/80001/build/android/pylib/symb... build/android/pylib/symbols/elf_symbolizer.py:359: failed_disambiguation = source_path is None I'm not sure that you need at all to keep failed_disambiguation and the failed_disambiguated argument. What you really might want to do, on the caller side, is checking how many file paths that you got from here exists in the filesystem (which means that you either disambiguated or stripped succesfully) https://codereview.chromium.org/339853004/diff/80001/build/android/pylib/symb... build/android/pylib/symbols/elf_symbolizer.py:362: if source_path is not None: nit: if source_path: https://codereview.chromium.org/339853004/diff/80001/build/android/pylib/symb... build/android/pylib/symbols/elf_symbolizer.py:363: # Strip the base path I think that lines 363-368 can be expressed with a more concise: source_path = re.sub('^' + self._symbolizer.strip_base_path, self._symbolizer.source_root_path or '', source_path) which given the nice names of the variable is wonderfully concise and self-explicative. https://codereview.chromium.org/339853004/diff/80001/build/android/pylib/symb... build/android/pylib/symbols/elf_symbolizer.py:370: source_path = os.path.abspath(source_path) Good catch but move to line 120 in __init__ where you assign the var.
> https://codereview.chromium.org/339853004/diff/80001/build/android/pylib/symb... > build/android/pylib/symbols/elf_symbolizer.py:118: self.disambiguate = > source_root_path is not None > You don't seem to make any use of self.disambiguate. > Can you just directly use source_root_path in the check. > Also, typically it's nicer to be concise in python, unless there is a functional > need for it (if foo vs if foo is not None). > In essence, remove 118, and turn line 122 into > if source_root_path: It is used at line 357, but the same substitution could be made there; I kept it to clarify the code > https://codereview.chromium.org/339853004/diff/80001/build/android/pylib/symb... > build/android/pylib/symbols/elf_symbolizer.py:182: def > _CreateDisambiguationTable(self, src_root_path): > Is there a reason why you couldn't use those 4 lines of code I pasted in the > previous comment? > This seems a bit redundant and unnecessarily verbose: you populate the table, > keep track of duplicates, and then remove the duplicates from the table I missed this part "if fname not in src_paths_index else None " in your comment. Will change to what you suggested. > https://codereview.chromium.org/339853004/diff/80001/build/android/pylib/symb... > build/android/pylib/symbols/elf_symbolizer.py:354: disambiguated = False > I think that Andrew suggested that the two bools are unnecessary? Yes he did, but he also said: "Edit: OK, you could also use failed_disambiguation (like in later code), and flip the meaning in the truth table above." Did I misinterpret what he meant? > https://codereview.chromium.org/339853004/diff/80001/build/android/pylib/symb... > build/android/pylib/symbols/elf_symbolizer.py:357: if source_path and not > source_path.startswith('/'): > I think this line is completely unnecessary. > Just look if you have the entry in the table. If you have it in the table, the > path must have been necessarily != None and a relative path. > I.s., this should just be > source_path = self._symbolizer.lookup_table.get(source_path) or source_path That would yield disambiguated = source_path is not None which would increase the counter event if we did not need any disambiguation at all. > https://codereview.chromium.org/339853004/diff/80001/build/android/pylib/symb... > build/android/pylib/symbols/elf_symbolizer.py:359: failed_disambiguation = > source_path is None > I'm not sure that you need at all to keep failed_disambiguation and the > failed_disambiguated argument. > What you really might want to do, on the caller side, is checking how many file > paths that you got from here exists in the filesystem (which means that you > either disambiguated or stripped succesfully) Awesome :)
On 2014/06/18 15:56:51, sebastianl wrote: > It is used at line 357, but the same substitution could be made there; I kept it > to clarify the code Ah, sorry missed that. It's fine then > > > https://codereview.chromium.org/339853004/diff/80001/build/android/pylib/symb... > > build/android/pylib/symbols/elf_symbolizer.py:354: disambiguated = False > > I think that Andrew suggested that the two bools are unnecessary? > Yes he did, but he also said: > "Edit: OK, you could also use failed_disambiguation (like in later code), and > flip the meaning in the truth table above." > Did I misinterpret what he meant? Don't know. andrewhayden@? By the way, reading your last comment here sounds like you don't need to keep track of disambiguation success (the booleans) at all. > https://codereview.chromium.org/339853004/diff/80001/build/android/pylib/symb... > > build/android/pylib/symbols/elf_symbolizer.py:357: if source_path and not > > source_path.startswith('/'): > > I think this line is completely unnecessary. > > Just look if you have the entry in the table. If you have it in the table, the > > path must have been necessarily != None and a relative path. > > I.s., this should just be > > source_path = self._symbolizer.lookup_table.get(source_path) or source_path > That would yield > disambiguated = source_path is not None > which would increase the counter event if we did not need any disambiguation at > all. Again, looks like you don't want to really bother doing all this stuff here just to extract two booleans to check if you disambiguated or not. Look for the existence of the file as we discussed, no? > https://codereview.chromium.org/339853004/diff/80001/build/android/pylib/symb... > > build/android/pylib/symbols/elf_symbolizer.py:359: failed_disambiguation = > > source_path is None > > I'm not sure that you need at all to keep failed_disambiguation and the > > failed_disambiguated argument. > > What you really might want to do, on the caller side, is checking how many > file > > paths that you got from here exists in the filesystem (which means that you > > either disambiguated or stripped succesfully) > Awesome :) Sounds like we have a deal so?
On 2014/06/24 11:11:57, Primiano Tucci wrote: > https://codereview.chromium.org/339853004/diff/80001/build/android/pylib/symb... > > > build/android/pylib/symbols/elf_symbolizer.py:354: disambiguated = False > > > I think that Andrew suggested that the two bools are unnecessary? > > Yes he did, but he also said: > > "Edit: OK, you could also use failed_disambiguation (like in later code), > and > > flip the meaning in the truth table above." > > Did I misinterpret what he meant? > Don't know. andrewhayden@? > By the way, reading your last comment here sounds like you don't need to keep > track of disambiguation success (the booleans) at all. My point with that last bit about failed_disambiguation was that I had suggested these two booleans: (was_ambiguous,dismabiguated) You could use (was_ambiguous, failed_disambiguation) alternatively. I think it's useful to count how many times we tried to disambiguate and then, when we did try, how many times we succeeded. This gives a measure of the quality of the results, and helps a user get a more accurate picture of how confident they can be in the precision.
On 2014/06/24 11:18:06, Andrew Hayden wrote: > On 2014/06/24 11:11:57, Primiano Tucci wrote: > > > > https://codereview.chromium.org/339853004/diff/80001/build/android/pylib/symb... > > > > build/android/pylib/symbols/elf_symbolizer.py:354: disambiguated = False > > > > I think that Andrew suggested that the two bools are unnecessary? > > > Yes he did, but he also said: > > > "Edit: OK, you could also use failed_disambiguation (like in later code), > > and > > > flip the meaning in the truth table above." > > > Did I misinterpret what he meant? > > Don't know. andrewhayden@? > > By the way, reading your last comment here sounds like you don't need to keep > > track of disambiguation success (the booleans) at all. > > My point with that last bit about failed_disambiguation was that I had suggested > these two booleans: (was_ambiguous,dismabiguated) > You could use (was_ambiguous, failed_disambiguation) alternatively. > > I think it's useful to count how many times we tried to disambiguate and then, > when we did try, how many times we succeeded. This gives a measure of the > quality of the results, and helps a user get a more accurate picture of how > confident they can be in the precision. I don't feel all too comfortable removing the booleans, just because of the last thing andrewhayden mentioned in his comment. We could however remove the failed_disambiguation, and only keep disambiguated. But in my opinion, which has changed since the last comment, I would like to keep track of (disambiguated,failed_disambiguation) in the ELFSymbol class. Due to the fact that it brings more clarity to the user when using the tool. What if the user doesn't know the fact that if disambiguated is false then he or she needs to check the existence of the file in order to know whether the symbol path was disambiguated or not? @andrewhayden: did you mean that instead of using (disambiguated, failed_disambiguation) to use either (was_ambiguous, disambiguated) or (was_ambiguous, failed_disambiguation) ?
On 2014/06/24 12:31:03, sebastianl wrote: > @andrewhayden: did you mean that instead of using (disambiguated, > failed_disambiguation) to use either (was_ambiguous, disambiguated) or > (was_ambiguous, failed_disambiguation) ? Yes, pick whichever one you wish: (was_ambiguous, disambiguated) or (was_ambiguous, failed_disambiguation), it does not matter to me.
> > I think it's useful to count how many times we tried to disambiguate and
then,
> > when we did try, how many times we succeeded. This gives a measure of the
> > quality of the results, and helps a user get a more accurate picture of how
> > confident they can be in the precision.
To be honest I feel that is exposing an implementation detail, making the API
looking very weird, which returns
{this is the symbol name, this is the path, no I didn't disambiguated the
symbol, yes I failed to disambiguate it (oh really?)}
and, again, those booleans don't convey anything more interesting than what can
be inferred looking at the just the path of the file.
Also, note that by looking at source_path.startswith('/') you're assuming you're
running this on POSIX. What if the path is C:\bla? That's why I was proposing
only looking at the presence in the LUT.
Anyways, I don't want to block you guys, so if you feel strongly in favor of
that go ahead in any form you prefer, including adding a third boolean
has_tried_to_disambiguate_really_really_hard_i_swear.
but please address the other comments in the last review.
Just another note:
without a test covering that logic, that code is already dead ;-)
On 2014/06/24 15:29:24, Primiano Tucci wrote:
> To be honest I feel that is exposing an implementation detail, making the API
> looking very weird, which returns
> {this is the symbol name, this is the path, no I didn't disambiguated the
> symbol, yes I failed to disambiguate it (oh really?)}
> and, again, those booleans don't convey anything more interesting than what
can
> be inferred looking at the just the path of the file.
Strongly disagree. I think the API should provide as much information as is
reasonably possible, and we should leave it to the tools to do the downstream
analysis. Having the booleans here means the tools don't need to repeat the same
logic to determine whether or not a path is or is not ambiguous. That's a good
thing, and I do believe that there's value in doing the counts. If you ask for
symbolization and find that 75% of the symbols were ambiguous, that's a serious
problem; if it's 10% ambiguous, that's probably much less so.
Since you've expressed a willingness to accept, I'll request outright that we
leave it in.
Using (disambiguation, was_ambiuous) instead of (disambiguation, failed_disambiguation) @primiano: I did not rename lookup_table to _lookup_table due to that this dictionary is accessed from another class, and should not in my eyes should be private. Would appreciate if you'd take a look at the code, and see if you can find anything more :)
Cool stuff. I've just one remaining functional concern on line 350 (See comment below). For the rest just nits / names and we're clear to go :) I appreciate the patience, now it looks much cleaner. Thanks https://codereview.chromium.org/339853004/diff/100001/build/android/pylib/sym... File build/android/pylib/symbols/elf_symbolizer.py (right): https://codereview.chromium.org/339853004/diff/100001/build/android/pylib/sym... build/android/pylib/symbols/elf_symbolizer.py:95: source_root_path: The path to the library source code. Which is used in Nit: s/. Which// https://codereview.chromium.org/339853004/diff/100001/build/android/pylib/sym... build/android/pylib/symbols/elf_symbolizer.py:98: full and absolute path. In some toolchains only the name of the source Uh, what is the difference between a "full" and a "absolute" path? Probably you want just one of the two? https://codereview.chromium.org/339853004/diff/100001/build/android/pylib/sym... build/android/pylib/symbols/elf_symbolizer.py:102: same name, disambiguation will fail. Can we reword this a bit and make it a bit more clear and concise? What about: "path to the source tree on the local machine. It has two purposes: 1. turn relative file names names into full paths (foo.c -> /source/root/path/foo.c) iff foo.c is not ambiguous (i.e. unique in the |source_root_path|) to workaround some toolchains limitations; 2. serves as the replacement base for |strip_base_path| (below)" https://codereview.chromium.org/339853004/diff/100001/build/android/pylib/sym... build/android/pylib/symbols/elf_symbolizer.py:103: strip_base_path: If this base path is found in the beginning of any "rebases the symbols source paths onto |source_root_path| (i.e. s/^strip_base_path/source_root_path/)." https://codereview.chromium.org/339853004/diff/100001/build/android/pylib/sym... build/android/pylib/symbols/elf_symbolizer.py:120: self.lookup_table = {} What about: self.disambiguation_table = {} # 'foo.c' -> '/abs/path/foo.c' "lookup_table" has the same value of saying: self.array = [] self.number = 3 :) not really explicative. Variables names should reflect the role they play, not the data structure used to implement them :) https://codereview.chromium.org/339853004/diff/100001/build/android/pylib/sym... build/android/pylib/symbols/elf_symbolizer.py:183: def _CreateDisambiguationTable(self): Just add a one line comment please: """Non-unique file names will result in None entries""" https://codereview.chromium.org/339853004/diff/100001/build/android/pylib/sym... build/android/pylib/symbols/elf_symbolizer.py:185: source_root_path = os.path.abspath(self.source_root_path) I'd do this on line 121 and save one line: self.source_root_path = os.path.abspath(source_root_path) https://codereview.chromium.org/339853004/diff/100001/build/android/pylib/sym... build/android/pylib/symbols/elf_symbolizer.py:347: if source_path and not source_path.startswith('/'): Probably you can just use: posixpath.isabs(source_path) here https://codereview.chromium.org/339853004/diff/100001/build/android/pylib/sym... build/android/pylib/symbols/elf_symbolizer.py:350: disambiguated = source_path is not None Just a doubt: at this point if a2l here returns 'foo.c' and you don't have foo.c in your disambiguation table, source_path becomes none. In essence, looks like you're trying to enrich the output here, but if you happen to fail, you end up reducing the quality of the output ('foo.c' -> None.) Don't you probably want to do something like: full_path = self._symbolizer.disambiguation_table.get(source_path) if full_path: source_path = full_path disambiguated = True ? https://codereview.chromium.org/339853004/diff/100001/build/android/pylib/sym... build/android/pylib/symbols/elf_symbolizer.py:355: self._symbolizer.source_root_path or '', source_path) Nit: indentation should be 4 spaces here, or inline. i.e. either source_path = re.sub('^' + self._symbolizer.strip_base_path, self._symbolizer.source_root_path or '', source_path) or source_path = re.sub('^' + self._symbolizer.strip_base_path, self._symbolizer.source_root_path or '', source_path) https://codereview.chromium.org/339853004/diff/100001/build/android/pylib/sym... build/android/pylib/symbols/elf_symbolizer.py:443: self.source_path = (None if source_path is None else why do you need this logic (the abs path) here? Didn't you cover that above? If the client didn't ask for disambiguation, he's fine to get relative paths. Also, this is conceptually wrong: if you have 'foo.c' (suppose you didn't chose disambiguation), abspath('foo.c') = PWD + 'foo.c', which is probably not what you want.
On 2014/06/25 09:02:59, Primiano Tucci wrote:
> Cool stuff.
> I've just one remaining functional concern on line 350 (See comment below).
> For the rest just nits / names and we're clear to go :)
>
> I appreciate the patience, now it looks much cleaner.
> Thanks
No worries, I have a lot of patience :)
I changed
source_path.startswith('/')
into
os.path.isabs(source_path)
instead of using posixpath, as os.path.isabs is OS independent (see os.path
documentation)
>
https://codereview.chromium.org/339853004/diff/100001/build/android/pylib/sym...
> build/android/pylib/symbols/elf_symbolizer.py:350: disambiguated = source_path
> is not None
> Just a doubt: at this point if a2l here returns 'foo.c' and you don't have
foo.c
> in your disambiguation table, source_path becomes none.
> In essence, looks like you're trying to enrich the output here, but if you
> happen to fail, you end up reducing the quality of the output ('foo.c' ->
None.)
> Don't you probably want to do something like:
>
> full_path = self._symbolizer.disambiguation_table.get(source_path)
> if full_path:
> source_path = full_path
> disambiguated = True
> ?
Makes a lot of sense to do this change. I never gave it a thought honestly :) I
wrote another variant of what you suggested above in my latest patch.
>
https://codereview.chromium.org/339853004/diff/100001/build/android/pylib/sym...
> build/android/pylib/symbols/elf_symbolizer.py:443: self.source_path = (None if
> source_path is None else
> why do you need this logic (the abs path) here? Didn't you cover that above?
If
> the client didn't ask for disambiguation, he's fine to get relative paths.
> Also, this is conceptually wrong: if you have 'foo.c' (suppose you didn't
chose
> disambiguation), abspath('foo.c') = PWD + 'foo.c', which is probably not what
> you want.
Removing it from the constructor of ELFSymbol makes sense. I instead changed so
that if disambiguation is enabled, and source_path is not None, then absolute
paths will be enforced. As some addr2line output may contain "/../../" variants
(ex. /home/someguy/dev/chromium/src/out/Release/../../somefile.cpp). This at
least enforces some kind of consistency when disambiguation is enabled (as the
disambiguation_table also contains absolute paths).
LGTM % 1 nit (posixpath) and a the abspath point (see below). For the abspath thing, feel free to do what you feel makes more sense. I am not strong about it, just let's be sure that this is what you actually intend to do. You got your green light ;-) > Makes a lot of sense to do this change. I never gave it a thought honestly :) I > wrote another variant of what you suggested above in my latest patch. I know, that was exactly the point. The check should be relative to the OS where you expect the elf was compiled from, not relative to the OS where you're running addr2line. If you're symbolizing an elf binary, it is extremely likely that the .so has been compiled on Linux. If you you symbolize that elf from windows, very likely its symbols path will be like /foo/bar, so you still want to check if their path is absolute in the Posix sense. ;-) >I instead changed so that if disambiguation is enabled, and source_path is not None, then absolute > paths will be enforced. As some addr2line output may contain "/../../" variants > (ex. /home/someguy/dev/chromium/src/out/Release/../../somefile.cpp). This at > least enforces some kind of consistency when disambiguation is enabled (as the > disambiguation_table also contains absolute paths). I am not sure you should do this. If the symbol was not ambiguous, these are the possible cases (correct me if I'm wrong): - The path was already absolute (regardless the fact that you were disambiguating or not). In this case abspath is a noop. - You were disambiguating, the disambiguation failed, so now you have source_path = 'foo.c'. In this case the right thing to propagate is 'foo.c'. If you abspath() it, you will make it absolute w.r.t of CWD (e.g. /home/primiano/foo.c), which is very likely to be nonsense. - The path is relative (like ../../home/foo). As before, this will end-up in a lie.
On 2014/06/25 12:54:01, Primiano Tucci wrote:
> LGTM % 1 nit (posixpath) and a the abspath point (see below).
>
> For the abspath thing, feel free to do what you feel makes more sense. I am
not
> strong about it, just let's be sure that this is what you actually intend to
do.
> You got your green light ;-)
>
Awesome
@andrewhayden: what do you think about the changes made to
run_binary_size_analysis.py ?
> > Makes a lot of sense to do this change. I never gave it a thought honestly
:)
> I
> > wrote another variant of what you suggested above in my latest patch.
>
> I know, that was exactly the point. The check should be relative to the OS
where
> you expect the elf was compiled from, not relative to the OS where you're
> running addr2line.
> If you're symbolizing an elf binary, it is extremely likely that the .so has
> been compiled on Linux.
> If you you symbolize that elf from windows, very likely its symbols path will
be
> like /foo/bar, so you still want to check if their path is absolute in the
Posix
> sense. ;-)
>
Using posixpath.isabs instead of os.path.isabs will exclude some disambiguations
from the results (unfortunately). I tested this by simple replacing
os.path.isabs with posixpath.isabs, and got more ambiguous paths
('/somefile.cc') in the result than if I used os.path.isabs. This makes me
wonder whether instead of using builtin path modules, that we instead should
just check whether the source_path starts with a '/' or not, as I did earlier :)
Note: I checked if the paths excluded were correct if using os.path.isabs
instead, and they were.
>
> >I instead changed so that if disambiguation is enabled, and source_path is
not
> None, then absolute
> > paths will be enforced. As some addr2line output may contain "/../../"
> variants
> > (ex. /home/someguy/dev/chromium/src/out/Release/../../somefile.cpp). This at
> > least enforces some kind of consistency when disambiguation is enabled (as
the
> > disambiguation_table also contains absolute paths).
>
> I am not sure you should do this.
> If the symbol was not ambiguous, these are the possible cases (correct me if
I'm
> wrong):
>
> - The path was already absolute (regardless the fact that you were
> disambiguating or not). In this case abspath is a noop.
>
> - You were disambiguating, the disambiguation failed, so now you have
> source_path = 'foo.c'. In this case the right thing to propagate is 'foo.c'.
If
> you abspath() it, you will make it absolute w.r.t of CWD (e.g.
> /home/primiano/foo.c), which is very likely to be nonsense.
Which is why the if-statement above states "if source_path and not
was_ambiguous" :)
as was_ambiguous is True whether the disambiguation failed or not (if it
succeeded, it will be an abspath anyways). So that we only change the relative
paths (ie. ../../home/foo => /home/foo) that was returned by addr2line, and
wasn't ambiguous.
>
> - The path is relative (like ../../home/foo). As before, this will end-up in
a
> lie.
If we run the symbolization from "/home/foo/" and get the relative path
"../../home/bar/" and disambiguation is enabled (source_root_path is not none),
then the result will end up as "/home/bar/" => this does not apply if
disambiguation is disabled :)
LGTM and thank you for your continued work! https://codereview.chromium.org/339853004/diff/120001/build/android/pylib/sym... File build/android/pylib/symbols/elf_symbolizer.py (right): https://codereview.chromium.org/339853004/diff/120001/build/android/pylib/sym... build/android/pylib/symbols/elf_symbolizer.py:95: source_root_path: The path to the library source code is used in Grammar here still needs to be cleaned up a bit. I think the original description I provided is sufficient here, so let's move your example to the end instead of the beginning of this text. How about this? In some toolchains only the name of the source file is output, without any path information; disambiguation searches through the source directory specified by |source_root_path| argument for files whose name matches, adding the full path information to the output. For example, if the toolchain outputs "unicode.cc" and there is a file called "unicode.cc" located under |source_root_path|/foo/bar, the tool will replace "unicode.cc" with "|source_root_path|/foo/bar/unicode.cc". If there are multiple files with the same name, disambiguation will fail because the tool cannot determine which of the files was the source of the symbol. https://codereview.chromium.org/339853004/diff/120001/build/android/pylib/sym... build/android/pylib/symbols/elf_symbolizer.py:104: (i.e. s/^strip_base_path/source_root_path/). This is a nice succinct explanation, though the use of search and replace syntax here may be confusing for folks who don't use Linux/Mac... but since it's an ELF symbolizer, I guess that's ok :P :P Still for clarity and accessibility to a wider audience you might want to provide an example. https://codereview.chromium.org/339853004/diff/120001/build/android/pylib/sym... build/android/pylib/symbols/elf_symbolizer.py:343: # In case disambiguation is on, and needed This loop has gotten a lot nicer. I think it's self-documenting now, which is cool. Nice job. https://codereview.chromium.org/339853004/diff/120001/build/android/pylib/sym... File build/android/pylib/symbols/elf_symbolizer_unittest.py (right): https://codereview.chromium.org/339853004/diff/120001/build/android/pylib/sym... build/android/pylib/symbols/elf_symbolizer_unittest.py:173: unittest.main() Looks like this file can be removed from the review. I don't see any change left, and I'm not sure why line 173 is highlighted by the review. EOL character or something? https://codereview.chromium.org/339853004/diff/120001/tools/binary_size/run_b... File tools/binary_size/run_binary_size_analysis.py (right): https://codereview.chromium.org/339853004/diff/120001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:745: ' NOTE: this will produce output with some symbols at the' "will" -> "may, depending upon your toolchain," https://codereview.chromium.org/339853004/diff/120001/tools/binary_size/run_b... tools/binary_size/run_binary_size_analysis.py:746: ' top layer due to the fact that addr2line could not get' "due to the fact that" -> "if"
LGTM and thank you for your continued work!
On 2014/06/26 08:35:20, sebastianl wrote: > On 2014/06/25 12:54:01, Primiano Tucci wrote: > > > > - The path is relative (like ../../home/foo). As before, this will end-up in > a > > lie. > If we run the symbolization from "/home/foo/" and get the relative path > "../../home/bar/" and disambiguation is enabled (source_root_path is not none), > then the result will end up as "/home/bar/" => this does not apply if > disambiguation is disabled :) Note that I landed a fix ( https://codereview.chromium.org/303453003/ ) a few days ago (that I though I landed 2 weeks ago but had forgotten) that makes sure relative paths in the elf_symbolizer output is resolved towards the build location rather than cwd. So you might want to rebase this and see if it changes more. It didn't seem like it would conflict in the code, but partly they are addressing the same issue, paths.
> Using posixpath.isabs instead of os.path.isabs will exclude some
disambiguations
> from the results (unfortunately). I tested this by simple replacing
> os.path.isabs with posixpath.isabs, and got more ambiguous paths
> ('/somefile.cc') in the result than if I used os.path.isabs.
Uh? Really? On which OS? On Linux os.path.isabs is exactly the same of
posixpath.isabs, as one would expect:
>>> os.path.isabs
<function isabs at 0x7f06918fede8>
>>> posixpath.isabs
<function isabs at 0x7f06918fede8>
>>> posixpath.isabs == os.path.isabs
True
> This makes me
> wonder whether instead of using builtin path modules, that we instead should
> just check whether the source_path starts with a '/' or not, as I did earlier
:)
Is that going to be different? If you use the .startswith('/') wouldn't you also
mistakenly assume that /foo.c is absolute?
> Note: I checked if the paths excluded were correct if using os.path.isabs
> instead, and they were.
Not sure I really follow here. I am curious at this point. Can you make some
examples of string which end up differently using posixpath.isabs, os.path.isabs
and .startswith('/') ?
> Which is why the if-statement above states "if source_path and not
> was_ambiguous" :)
> as was_ambiguous is True whether the disambiguation failed or not (if it
> succeeded, it will be an abspath anyways). So that we only change the relative
> paths (ie. ../../home/foo => /home/foo) that was returned by addr2line, and
> wasn't ambiguous.
I don't really follow. If CWD = /home/primiano/chrome/src and path =
'../../home/foo'
the result will be /home/primiano/home/foo. This is what you want?
> If we run the symbolization from "/home/foo/" and get the relative path
> "../../home/bar/" and disambiguation is enabled (source_root_path is not
none),
> then the result will end up as "/home/bar/"
And this is the intended behavior? You're essentially telling me that the
behavior of this module is dependent on the CWD. Or am I misunderstanding here?
https://codereview.chromium.org/339853004/diff/120001/build/android/pylib/sym... File build/android/pylib/symbols/elf_symbolizer.py (right): https://codereview.chromium.org/339853004/diff/120001/build/android/pylib/sym... build/android/pylib/symbols/elf_symbolizer.py:343: # In case disambiguation is on, and needed > I think it's self-documenting now, which is cool. Really? is it? I think that nobody here is really understanding all the cases where the path is kept as it is vs. made absolute (and w.r.t of which base dir). What would be self-documenting, instead, would be code in the unittests which clearly defines the possible input and the output expectations. Just to be clear, this still LGTM. I just give up trying to follow the path logic here. Similarly, as a reviewer, I will not encourage any developer, who will happen to touch this code in future, to spend any time trying to reverse engineer the intended behavior here. This is the very first Chrome dev principle: there is just code which is covered by tests and code which is broken.
On 2014/06/26 09:33:45, Primiano Tucci wrote: > https://codereview.chromium.org/339853004/diff/120001/build/android/pylib/sym... > File build/android/pylib/symbols/elf_symbolizer.py (right): > > https://codereview.chromium.org/339853004/diff/120001/build/android/pylib/sym... > build/android/pylib/symbols/elf_symbolizer.py:343: # In case disambiguation is > on, and needed > > I think it's self-documenting now, which is cool. > Really? is it? > > I think that nobody here is really understanding all the cases where the path is > kept as it is vs. made absolute (and w.r.t of which base dir). > What would be self-documenting, instead, would be code in the unittests which > clearly defines the possible input and the output expectations. > > Just to be clear, this still LGTM. I just give up trying to follow the path > logic here. Similarly, as a reviewer, I will not encourage any developer, who > will happen to touch this code in future, to spend any time trying to reverse > engineer the intended behavior here. > > This is the very first Chrome dev principle: there is just code which is covered > by tests and code which is broken. I think unit tests that show the desired output are a great idea.
PS - I think an even better idea is to get this out of the linker and get as far away as possible from nm :)
On 2014/06/26 09:21:41, Primiano Tucci wrote:
> > Using posixpath.isabs instead of os.path.isabs will exclude some
> disambiguations
> > from the results (unfortunately). I tested this by simple replacing
> > os.path.isabs with posixpath.isabs, and got more ambiguous paths
> > ('/somefile.cc') in the result than if I used os.path.isabs.
>
> Uh? Really? On which OS? On Linux os.path.isabs is exactly the same of
> posixpath.isabs, as one would expect:
>
> >>> os.path.isabs
> <function isabs at 0x7f06918fede8>
> >>> posixpath.isabs
> <function isabs at 0x7f06918fede8>
> >>> posixpath.isabs == os.path.isabs
> True
This had me confused as well, until I reran my tests and the results was correct
using both. Must've made an error before :)
> > Which is why the if-statement above states "if source_path and not
> > was_ambiguous" :)
> > as was_ambiguous is True whether the disambiguation failed or not (if it
> > succeeded, it will be an abspath anyways). So that we only change the
relative
> > paths (ie. ../../home/foo => /home/foo) that was returned by addr2line, and
> > wasn't ambiguous.
> I don't really follow. If CWD = /home/primiano/chrome/src and path =
> '../../home/foo'
> the result will be /home/primiano/home/foo. This is what you want?
>
> > If we run the symbolization from "/home/foo/" and get the relative path
> > "../../home/bar/" and disambiguation is enabled (source_root_path is not
> none),
> > then the result will end up as "/home/bar/"
> And this is the intended behavior? You're essentially telling me that the
> behavior of this module is dependent on the CWD. Or am I misunderstanding
here?
Well, yes. If disambiguation is enabled, all paths disambiguated will be
absolute; it's just an assurance that the paths returned by addr2line that isn't
ambiguous ones also is absolute.
@bratell: I will rebase and see if they're compatible :)
Hopefully the last patch. * I rebased to master in order to get bratells changes made to the binary_size_tool * Updated descriptions according to andrewhaydens recommendations * Replaced "os.path.isabs" with "posixpath.isabs" according to primianos recommendations Do you guys have any more comments before I continue? :)
Still LGTM https://codereview.chromium.org/339853004/diff/120001/build/android/pylib/sym... File build/android/pylib/symbols/elf_symbolizer.py (right): https://codereview.chromium.org/339853004/diff/120001/build/android/pylib/sym... build/android/pylib/symbols/elf_symbolizer.py:104: (i.e. s/^strip_base_path/source_root_path/). On 2014/06/26 09:00:11, Andrew Hayden wrote: > This is a nice succinct explanation, though the use of search and replace syntax > here may be confusing for folks who don't use Linux/Mac... but since it's an ELF > symbolizer, I guess that's ok :P :P > > Still for clarity and accessibility to a wider audience you might want to > provide an example. I find the idea that a developer is not familiar with the regex syntax terrifying, but I'm fine either ways ;-)
The CQ bit was checked by sebastianl@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sebastianl@opera.com/339853004/140001
Message was sent while issue was closed.
Change committed as 280303 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
