Mpf get str fixups #25

bryant · 2017-06-07T00:54:05Z

bug was introduced in 877104f.

the intent here is to allocate enough space for gmpf_get_str to plop a string. if all digits are requested (n_digits == 0), we defer to gmpf_get_str itself to figure out how much space is needed (because the limb nonsense is too tedious to deal with) by passing nullptr to the first argument, as documented in the manual:

If str is NULL, the result string is allocated using the current allocation function (see Custom Allocation). The block will be strlen(str)+1 bytes, that being exactly enough for the string and null-terminator.

if the number of digits requested is known (non-zero n_digits), then simply allocate a correctly sized cstring (unlike 877104f, which allocates a 0-byte string).

to avoid memory leaks from repeated get_str(n_digits=0) calls, a wrapper is used to de-allocate on drop the gmp-allocated string.

with this patch, #24 passes.

from https://gmplib.org/manual/Converting-Floats.html : `Function: char * mpf_get_str (char *str, mp_exp_t *expptr, int base, size_t n_digits, const mpf_t op)`

andersk · 2017-06-09T09:51:57Z

src/mpf.rs

+            }
+        } else {
+            // From mpf/get_str.c.
+            let c_str = CString::new(vec![37; n_digits as usize + 2]).unwrap();


This intermediate CString is just overhead—why not use Vec<c_char>::as_ptr directly?

andersk · 2017-06-09T09:52:01Z

src/mpf.rs

+            let c_str = CString::new(vec![37; n_digits as usize + 2]).unwrap();
+            unsafe {
+                let p = __gmpf_get_str(c_str.as_ptr(), exp, base, n_digits, &self.mpf);
+                CString::from_raw(p).to_str().unwrap().to_string()


Doesn’t this double-free the string, once via c_str and once via the CString? I think you want CStr::from_ptr.

andersk · 2017-06-09T10:00:13Z

src/ffi.rs

+
+    pub fn to_str(&self) -> Result<&str, str::Utf8Error> {
+        let bytes: &[u8] = unsafe { slice::from_raw_parts(self.0, self.1) };
+        str::from_utf8(&bytes[..bytes.len() - 1])


CStr::from_ptr is more direct and avoids the separate strlen.

no matter what, the length needs to be known in order to call the free func, so it's either strlen or CStr::to_bytes().len(). i think strlen communicates more clearly how the length param to free_func is computed, whereas the alternate method relies on knowing that CStr implicitly calls strlen behind the scenes.

andersk · 2017-06-09T18:53:27Z

src/mpf.rs

+        } else {
+            // From mpf/get_str.c.
+            let bytes = unsafe {
+                let mut bytes: Vec<u8> = uninitialized();


You need a Vec with uninitialized data, not an uninitialized Vec! I think Vec::with_capacity is the right thing.

ugh, thanks for catching this.

andersk · 2017-06-09T19:08:10Z

src/mpf.rs

+            // From mpf/get_str.c.
+            let bytes = unsafe {
+                let mut bytes: Vec<u8> = uninitialized();
+                let c_str: *mut c_char = transmute(bytes.as_mut_ptr());


No need for a worrying transmute—you can just bytes.as_mut_ptr() as *mut c_char.

pretty sure that as_mut_ptr will return a * mut u8.

andersk · 2017-06-09T19:19:25Z

src/mpf.rs

+                __gmpf_get_str(c_str, exp, base, n_digits, &self.mpf);
+                bytes
+            };
+            String::from_utf8(bytes).unwrap()


That includes the trailing NUL, right?

yep. from mpf/get_str.c:

if (dbuf == 0) { /* We didn't get a string from the user. Allocate one (and return a pointer to it) with space for `-' and terminating null. */ alloc_size = n_digits + 2; dbuf = (char *) (*__gmp_allocate_func) (n_digits + 2);

andersk · 2017-06-09T19:34:29Z

src/mpf.rs

+        } else {
+            let bytes = unsafe {
+                // n_digits + 2 from mpf/get_str.c.
+                let mut bytes: Vec<u8> = vec![uninitialized(); n_digits as usize + 2];


This create a single uninitialized byte, then copies it n_digits + 2 times, so you haven’t really saved any work.

I think what you want is

let mut buf = Vec::with_capacity(n_digits as usize + 2); let ptr = __gmpf_get_str(buf.as_mut_ptr(), exp, base, n_digits, &self.mpf); CStr::from_ptr(ptr).to_str().unwrap().to_string()

(prior art)

i think i'll revert to vec![0; n_digits + 2], which will call rust_allocate_zeroed which i'm guessing is more or less calloc. the with_capacity trick is neat but seems a little unsafe, and to be honest, i'm not sure that enough digits are requested for an appreciable amount of work to be saved.

andersk · 2017-06-09T19:36:44Z

src/mpf.rs

+                __gmpf_get_str(c_str, exp, base, n_digits, &self.mpf);
+                bytes
+            };
+            String::from_utf8(bytes).unwrap()


No no, my point here was that String::from_utf8 includes the trailing NUL in its return value.

yeah, i realized what you meant a few minutes after hitting send. this branch should be tested anyway, and it shows up in the test.

this incorrectly created an uninitialized Vec, and using uninitialized isn't worth it anyway.

andersk · 2017-06-09T23:20:29Z

src/ffi.rs

+        use std::ptr::null_mut;
+        unsafe {
+            let mut free_func: FreeFunc = mem::uninitialized();
+            __gmp_get_memory_functions(null_mut(), null_mut(), mem::transmute(&mut free_func));


This transmute is unnecessary because &mut FreeFunc converts implicitly to *mut FreeFunc.

fizyk20 · 2017-09-03T05:26:56Z

Looks like this and #27 attempt to fix the same problem. I merged #27 before I had a look at your PR and now I think I might have done this too hastily. Your solution looks quite a bit more complex - do you see any obvious problems with how #27 attempts to do this?

bryant added 3 commits June 6, 2017 20:43

add wrapper for gmp-allocated strings

2370a3c

__gmpf_get_str is const on last arg.

4d797ca

from https://gmplib.org/manual/Converting-Floats.html : `Function: char * mpf_get_str (char *str, mp_exp_t *expptr, int base, size_t n_digits, const mpf_t op)`

properly call mpf_get_str

e2c9bde

bryant force-pushed the mpf-get-str-fixups branch from 1113816 to e2c9bde Compare June 7, 2017 01:00

bryant mentioned this pull request Jun 7, 2017

Mpf fixups #24

Open

andersk suggested changes Jun 9, 2017

View reviewed changes

bryant added 3 commits June 9, 2017 07:35

mpf_get_str's first arg is mut.

0ebc599

rework the known n_digits case.

f90d082

pass null_mut

68108f5

andersk reviewed Jun 9, 2017

View reviewed changes

andersk suggested changes Jun 9, 2017

View reviewed changes

initialize the Vec with zeros.

12cf1c9

this incorrectly created an uninitialized Vec, and using uninitialized isn't worth it anyway.

bryant force-pushed the mpf-get-str-fixups branch from 851473b to 9483bf6 Compare June 9, 2017 21:45

andersk reviewed Jun 9, 2017

View reviewed changes

bryant added 3 commits June 9, 2017 23:42

replace use of transmute with safe cast.

d820198

don't include null bytes from mpf_get_str.

3bcc529

test get_str.

58d47c2

bryant force-pushed the mpf-get-str-fixups branch from 9483bf6 to 58d47c2 Compare June 10, 2017 03:45

Mpf get str fixups #25

Are you sure you want to change the base?

Mpf get str fixups #25

Uh oh!

Conversation

bryant commented Jun 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andersk Jun 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andersk Jun 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fizyk20 commented Sep 3, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

andersk Jun 9, 2017 •

edited

Loading

andersk Jun 9, 2017 •

edited

Loading