bugGNU Octave - Bugs: bug #52810, regexprep: incorrect empty tokens...

 
 

bug #52810: regexprep: incorrect empty tokens on ARM architecture

Submitter:  Colin Macdonald <cbm>
Submitted:  Thu 04 Jan 2018 07:28:06 PM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * 4.2.1 Operating System:  * GNU/Linux
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Tue 16 Jan 2018 12:31:20 AM UTC, comment #10: 

I pushed your test here: http://hg.savannah.gnu.org/hgweb/octave/rev/4a4a8b2a5bf2.  I also backported the change to stable so that it will be part of 4.2.2.

Rik <rik5>
Group administrator
Mon 15 Jan 2018 06:49:40 PM UTC, comment #9: 

(Can I also request that we cherry-pick this bug fix onto stable?)

Colin Macdonald <cbm>
Mon 15 Jan 2018 06:45:39 PM UTC, comment #8: 

Thanks for the explanation.

I've attached a change which adds the BIST.


(file #42917)

Colin Macdonald <cbm>
Mon 15 Jan 2018 04:20:17 PM UTC, comment #7: 

I checked in the change here (http://hg.savannah.gnu.org/hgweb/octave/rev/845ec6f4fb96).  It would be useful if you could adapt mwe.m as a BIST test.  Make sure to use


%!test <*52810>


at the start of the test so it will be tagged as a regression if it ever starts failing again.

Regarding conversion of the post-processing to an m-file, core Octave has rather different constraints than the average user code.  In the case of the core, we often do choose performance over other goals.  In this case, the code is not amenable to conversion to an m-file because it contains a nested for loop over 1) matches, and 2) tokens.  Loops in an interpreted language are just very slow.

Rik <rik5>
Group administrator
Mon 15 Jan 2018 05:20:24 AM UTC, comment #6: 

I've tested it on ARM and it fixes the problem, thanks!

We should have a test right?  I can prepare a "#!test" version of my mwe.m

I agree with this change, but I wonder if rewriting regexprep as an m-file (postprocessing regexp output) would be more maintainable going forward.  I think Octave has too much C++ code (e.g., its a barrier in converting users to devs).

OTOH, maybe someone will be annoyed if regexprep is a bit slower.  And I'm not sure I have time to write such a patch.

Just my $0.02.

Colin Macdonald <cbm>
Sat 13 Jan 2018 12:32:19 AM UTC, comment #5: 

Attached is a changeset which I believe will fix the problem.  Can you apply it and compile from scratch on an ARM architecture to verify?  It doesn't lead to any test failures for 'make check' on an Intel platform.

(file #42898)

Rik <rik5>
Group administrator
Sat 06 Jan 2018 02:59:43 PM UTC, comment #4: 

Good sleuthing.  I'll try and take a look at this tomorrow.

Rik <rik5>
Group administrator
Sat 06 Jan 2018 07:40:41 AM UTC, comment #3: 

I found at least one problem!  And it is our fault.


  ...
  if (tokens[j].num == 0)
    pairlen += static_cast<size_t> (end - start) + 1;
  else if (tokens[j].num <= pairs.rows ())
    pairlen += static_cast<size_t> (pairs(tokens[j].num-1,1)
                                    - pairs(tokens[j].num-1,0)) + 1
  ...


Here pairs(...) - pairs(...) can be 45 - 46 = -1.  But then it (I guess) is cast to something unsigned, giving 0 (on ARM anyway).  Which then has 1 added to it.

I changed this code to:

  ...
  if (tokens[j].num == 0)
    pairlen += static_cast<size_t> (end - start + 1);
  else if (tokens[j].num <= pairs.rows ())
    pairlen += static_cast<size_t> (pairs(tokens[j].num-1,1)
                                    - pairs(tokens[j].num-1,0) + 1)
  ...

And at least pairlen became correct.

But I see several other instances of static_cast<size_t> on double subtractions which look suspicious.

But maybe the real question is why are double being used as indices into strings here?

Colin Macdonald <cbm>
Fri 05 Jan 2018 07:49:50 AM UTC, comment #2: 

I dug a little deeper and I think it might be an Octave bug.

1.  First, note (as in mwe.m) that "regexp" does the right thing here.  Its "regexprep" that shows the bug.

2.  My reading of regexp::replace in lo-regexp.cc is that it calls regexp::match (which uses PCRE) but then does its own substitutions.

Sorry this is not as ironclad as your suggestion but its all I have time to learn at the moment.

Colin Macdonald <cbm>
Thu 04 Jan 2018 07:32:38 PM UTC, comment #1: 

Octave use the library PCRE for regular expressions.  My guess is that the problem is with the underlying library on ARM architectures.  Although, if you can generate a test case in C or C++ using the PCRE library directly that doesn't show the problem then we could look further inside Octave for an answer.  But, probably this is an upstream bug.

Rik <rik5>
Group administrator
Thu 04 Jan 2018 07:28:06 PM UTC, original submission:  

When a token should be empty, it sometimes contains a single character (which seems to come from the next token).  This happens on ARM but not on x86.

I will attach a mwe.m of the following example.

Consider a string like:

s1 =
abcdef
            Xkey
abcdef
 Xkey
abcdef
abcdef
Xkey
abcdef



%% find <whitespace>Xkey and put some underscores in
s2 = regexprep(s1, '^([ \t]*)(Xkey)$', '___$1___$2___', ...
               "dotexceptnewline", "emptymatch", "lineanchors")


On x86, we get:

s2 =
abcdef
___            ___Xkey___
abcdef
___ ___Xkey___
abcdef
abcdef
______Xkey___
abcdef


On ARM we get a different and wrong result:

s2 =
abcdef
___            ___Xkey___
abcdef
___ ___Xkey___
abcdef
abcdef
___X___Xkey___
abcdef

Note that extra "X" on the penultimate line.  "$1" is incorrect.

I've hit this or very similar bugs on earlier version of Octave (at least 4.0).  I don't have 4.3.0+ built on ARM but I suspect its still present there too.

Colin Macdonald <cbm>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #42917:  52810_test.cset added by cbm (793B - application/octet-stream)
file #42898:  52810.cset added by rik5 (3KiB - application/octet-stream)
file #42833:  mwe.m added by cbm (645B - text/x-objcsrc)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by cbm (Submitted the item)
  • -email is unavailable- added by cbm
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-01-15 cbm Attached File- Added 52810_test.cset, #42917
    2018-01-15 rik5 StatusConfirmed Fixed
        Open/ClosedOpen Closed
    2018-01-13 rik5 Attached File- Added 52810.cset, #42898
    2018-01-06 rik5 StatusNone Confirmed
    2018-01-04 cbm Attached File- Added mwe.m, #42833
        Carbon-Copy- Added oheim

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code