LLVM Bugzilla is read-only and represents the historical archive of all LLVM issues filled before November 26, 2021. Use github to submit LLVM bugs

Bug 32137 - tools/bugpoint/ToolRunner.cpp:386: bad for loop ?
Summary: tools/bugpoint/ToolRunner.cpp:386: bad for loop ?
Status: RESOLVED INVALID
Alias: None
Product: tools
Classification: Unclassified
Component: bugpoint (show other bugs)
Version: trunk
Hardware: PC Linux
: P normal
Assignee: Unassigned LLVM Bugs
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-03-04 02:47 PST by David Binderman
Modified: 2019-04-13 07:18 PDT (History)
4 users (show)

See Also:
Fixed By Commit(s):


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description David Binderman 2017-03-04 02:47:43 PST
tools/bugpoint/ToolRunner.cpp:386]: (error) When Pos==CommandLine.size(), CommandLine[Pos] is out of bounds.

Source code is

  for (std::size_t Pos = 0u; Pos <= CommandLine.size(); ++Pos) {
    if ('\\' == CommandLine[Pos]) {

Maybe better code

  for (std::size_t Pos = 0u; Pos < CommandLine.size(); ++Pos) {
    if ('\\' == CommandLine[Pos]) {
Comment 1 Andrea Di Biagio 2017-03-07 10:28:39 PST
Hi David,

Thanks for reporting this issue.
Owen just uploaded a patch for it here: https://reviews.llvm.org/D30704

Could you please verify if that fixes your issue?

Thanks,
Andrea
Comment 2 David Binderman 2017-03-07 10:52:36 PST
>Could you please verify if that fixes your issue?

No it doesn't. Proposed solution misses the point.

For loop still has inclusive upper and lower limits,
which is IMHO a newbie off by one error. 

Every C programmer should know that lower limits are inclusive and 
upper limits are exclusive.

Suggest have another look at my proposed solution.
Comment 3 Andrea Di Biagio 2017-03-07 11:29:48 PST
(In reply to David Binderman from comment #2)
> >Could you please verify if that fixes your issue?
> 
> No it doesn't. Proposed solution misses the point.
> 
> For loop still has inclusive upper and lower limits,
> which is IMHO a newbie off by one error. 
> 
> Every C programmer should know that lower limits are inclusive and 
> upper limits are exclusive.
> 
> Suggest have another look at my proposed solution.

If you think that his patch is not okay, then perhaps leave comment on the code review. I posted that message on Owen's behalf since he didn't have a bugzilla account and he had to leave the office.
Comment 4 Edd Dawson 2017-03-13 13:13:49 PDT
I'm writing this on Owen's behalf. He requested a bugzilla login a few days ago, but it hasn't yet been created.

----

Hi David,

Thanks for looking into this revision, after looking further into the issue you describe I have found that a fix is not required. 

Unfortunately your suggested change will break the algorithm. The final pass in the loop is required so a remaining token can be pushed into the Args vector (line 392 to 406). As the bug regards access using the [] operator I uploaded a new revision, however I have abandoned this change due to the following in the C++11 standard:

21.4.5 basic_string element access
operator[](size_type pos);
Requires: pos <= size().
Returns: *(begin() + pos) if pos < size(), otherwise a reference to an object of type T with value charT(); the referenced value shall not be modified.

This means that the operator[str.size()] will return '\0'

Looking at your first post it appears to include an error message. Could you please describe the method used to find this? 

Thanks,

Owen
Comment 5 David Binderman 2017-03-14 00:48:24 PDT
(In reply to Edd Dawson from comment #4)
> This means that the operator[str.size()] will return '\0'

News to me. Thanks for the information.

> Looking at your first post it appears to include an error message. Could you
> please describe the method used to find this? 

I used cppcheck, a C/C++ static analyser.

http://cppcheck.sourceforge.net/

I find it very useful.
Comment 6 Simon Pilgrim 2019-04-13 07:18:18 PDT
Overzealous cppcheck warning