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

 
 

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

Submitted by:  Colin Macdonald <cbm>
Submitted on:  Thu 04 Jan 2018 07:28:06 PM UTC  
 
Category: Octave FunctionSeverity: 3 - Normal
Priority: 5 - NormalItem Group: Incorrect Result
Status: FixedAssigned to: None
Originator Name: Open/Closed: Closed
Release: 4.2.1Operating System: GNU/Linux

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission 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>
Project 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

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>
Project 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>
Project 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>
Project Administrator
Sat 06 Jan 2018 07:40:41 AM UTC, comment #3:

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

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:
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>
Project 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:

On x86, we get:

On ARM we get a different and wrong result:
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 File(s):
   
   
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
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 7 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Mon 15 Jan 2018 06:45:39 PM UTCcbmAttached File-=>Added 52810_test.cset, #42917
    Mon 15 Jan 2018 04:20:17 PM UTCrik5StatusConfirmed=>Fixed
      Open/ClosedOpen=>Closed
    Sat 13 Jan 2018 12:32:19 AM UTCrik5Attached File-=>Added 52810.cset, #42898
    Sat 06 Jan 2018 02:59:43 PM UTCrik5StatusNone=>Confirmed
    Thu 04 Jan 2018 07:28:06 PM UTCcbmAttached File-=>Added mwe.m, #42833
      Carbon-Copy-=>Added oheim

    Back to the top


    Powered by Savane 3.1-cleanup1