Fixed google::FindSymbol reading past end of a section

Fixed google::FindSymbol reading past end of a section

The symbol reading logic of google::FindSymbol was reading symbols in
blocks of N, not accounting for that a section might not have a multiple
of N symbols in it.

This makes it read in blocks of N or the number of symbols remaining,
whichever is smallest.

BUG=672481
Review-Url: https://codereview.chromium.org/2566623003
Cr-Commit-Position: refs/heads/master@{#443571}
Committed: https://chromium.googlesource.com/chromium/src/+/3dae0a2d7da309159a301e40a592692ec8431a38

[bjornr, 2016-12-09 10:14:20]

Description was changed from

==========
Fixed google::FindSymbol reading past end of a section

The symbol reading logic of google::FindSymbol was reading symbols in
blocks of N, not accounting for that a section might not have a multiple
of N symbols in it.

This makes it read in blocks of N or the number of symbols remaining,
whichever is smallest.

BUG=672481
==========

to

==========
Fixed google::FindSymbol reading past end of a section

The symbol reading logic of google::FindSymbol was reading symbols in
blocks of N, not accounting for that a section might not have a multiple
of N symbols in it.

This makes it read in blocks of N or the number of symbols remaining,
whichever is smallest.

BUG=672481
==========

[bjornr, 2016-12-09 10:14:21]

bjornr@opera.com changed reviewers:
+ satorux@chromium.org

[bjornr, 2016-12-09 10:16:17]

@satorux1 does this look ok to you?

[satorux1, 2016-12-16 06:54:20]

satorux@chromium.org changed reviewers:
+ hamaji@chromium.org

[satorux1, 2016-12-16 06:54:21]

+hamaji

sorry for the belated response. +hamaji could you take a look? though I wrote
the original code, my memory with the symbol table business is rusted...

[hamaji, 2016-12-16 07:21:14]

hamaji@google.com changed reviewers:
+ hamaji@google.com

[hamaji, 2016-12-16 07:21:14]

LGTM. Apparently internal version does the same

https://codereview.chromium.org/2566623003/diff/1/base/third_party/symbolize/...
File base/third_party/symbolize/symbolize.cc (right):

https://codereview.chromium.org/2566623003/diff/1/base/third_party/symbolize/...
base/third_party/symbolize/symbolize.cc:299: int num_symbols_to_read =
num_symbols - i;
Use std::min?

[bjornr, 2016-12-16 10:34:02]

bjornr@opera.com changed reviewers:
- hamaji@google.com

[bjornr, 2016-12-21 12:44:31]

The CQ bit was checked by bjornr@opera.com

[bjornr, 2016-12-21 12:44:31]

The patchset sent to the CQ was uploaded after l-g-t-m from hamaji@google.com
Link to the patchset: https://codereview.chromium.org/2566623003/#ps20001
(title: "Use std::min")

[commit-bot: I haz the power, 2016-12-21 12:44:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-21 12:44:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-21 12:44:43]

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[bjornr, 2016-12-21 12:44:58]

The CQ bit was unchecked by bjornr@opera.com

[bjornr, 2016-12-21 12:53:19]

On 2016/12/16 07:21:14, hamaji wrote:
> LGTM. Apparently internal version does the same
> 
>
https://codereview.chromium.org/2566623003/diff/1/base/third_party/symbolize/...
> File base/third_party/symbolize/symbolize.cc (right):
> 
>
https://codereview.chromium.org/2566623003/diff/1/base/third_party/symbolize/...
> base/third_party/symbolize/symbolize.cc:299: int num_symbols_to_read =
> num_symbols - i;
> Use std::min?

@hamaji Does this fixup look ok?

Sidenote: First chromium review so sorry if I'm accidentally generating spam or
something :X

[hamaji, 2016-12-23 08:57:23]

The CQ bit was checked by hamaji@chromium.org

[hamaji, 2016-12-23 08:57:24]

lgtm

LGTM. Sorry I confused you. I LGTM-ed from @google.com account, not from
@chromium.org.

[commit-bot: I haz the power, 2016-12-23 08:57:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-23 09:02:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-23 09:02:53]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[bjornr, 2017-01-02 10:20:49]

bjornr@opera.com changed reviewers:
+ mark@chromium.org

[bjornr, 2017-01-02 10:23:11]

@mark Does this look ok? (alrdy reviewed by authors, need approval from owner)

[Mark Mentovai, 2017-01-03 17:39:16]

LGTM. But the README.chromium says that symbolize.cc is an as-is copy of
something upstream. That’s no longer true, so README.chromium should be fixed.

Better yet, get this fix committed upstream and update our copy.

[bjornr, 2017-01-13 15:32:56]

The CQ bit was checked by bjornr@opera.com

[commit-bot: I haz the power, 2017-01-13 15:33:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bjornr, 2017-01-13 15:37:59]

On 2017/01/03 17:39:16, Mark Mentovai wrote:
> LGTM. But the README.chromium says that symbolize.cc is an as-is copy of
> something upstream. That’s no longer true, so README.chromium should be fixed.
> 
> Better yet, get this fix committed upstream and update our copy.

I'll land this first as I don't have much time, and the README.chromium was
already wrong before this patch (not saying it's good, but at least I'm not
breaking it by landing this).

[commit-bot: I haz the power, 2017-01-13 16:30:01]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484321576608430,
"parent_rev": "881daca50cda844224e2d15600b47aba179c090d", "commit_rev":
"3dae0a2d7da309159a301e40a592692ec8431a38"}

[commit-bot: I haz the power, 2017-01-13 16:30:36]

Description was changed from

==========
Fixed google::FindSymbol reading past end of a section

The symbol reading logic of google::FindSymbol was reading symbols in
blocks of N, not accounting for that a section might not have a multiple
of N symbols in it.

This makes it read in blocks of N or the number of symbols remaining,
whichever is smallest.

BUG=672481
==========

to

==========
Fixed google::FindSymbol reading past end of a section

The symbol reading logic of google::FindSymbol was reading symbols in
blocks of N, not accounting for that a section might not have a multiple
of N symbols in it.

This makes it read in blocks of N or the number of symbols remaining,
whichever is smallest.

BUG=672481

Review-Url: https://codereview.chromium.org/2566623003
Cr-Commit-Position: refs/heads/master@{#443571}
Committed:
https://chromium.googlesource.com/chromium/src/+/3dae0a2d7da309159a301e40a592...
==========

[commit-bot: I haz the power, 2017-01-13 16:30:37]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/3dae0a2d7da309159a301e40a592...

[Nico, 2017-05-02 15:15:10]

On 2017/01/13 15:37:59, bjornr wrote:
> On 2017/01/03 17:39:16, Mark Mentovai wrote:
> > LGTM. But the README.chromium says that symbolize.cc is an as-is copy of
> > something upstream. That’s no longer true, so README.chromium should be
fixed.
> > 
> > Better yet, get this fix committed upstream and update our copy.
> 
> I'll land this first as I don't have much time, and the README.chromium was
> already wrong before this patch (not saying it's good, but at least I'm not
> breaking it by landing this).

Can you at least fix the readme?