Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add natvis for Result, NonNull, CString, CStr, and Cow #82557

Merged
merged 1 commit into from
Mar 8, 2021
Merged

Add natvis for Result, NonNull, CString, CStr, and Cow #82557

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/etc/natvis/liballoc.natvis
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,4 +75,11 @@
<ExpandedItem>ptr.pointer->data</ExpandedItem>
</Expand>
</Type>
<Type Name="alloc::borrow::Cow&lt;*&gt;">
<DisplayString Condition="RUST$ENUM$DISR == 0x0">Borrowed({__0})</DisplayString>
<DisplayString Condition="RUST$ENUM$DISR == 0x1">Owned({__0})</DisplayString>
<Expand>
<Item Name="[value]" ExcludeView="simple">__0</Item>
</Expand>
</Type>
</AutoVisualizer>
15 changes: 15 additions & 0 deletions src/etc/natvis/libcore.natvis
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,4 +30,19 @@
</Expand>
</Type>

<Type Name="core::result::Result&lt;*&gt;">
<DisplayString Condition="RUST$ENUM$DISR == 0x0">Ok({__0})</DisplayString>
<DisplayString Condition="RUST$ENUM$DISR == 0x1">Err({(*($T2*) &amp;__0)})</DisplayString>
<Expand>
<Item Name="[value]" Condition="RUST$ENUM$DISR == 0x0">__0</Item>
<Item Name="[value]" Condition="RUST$ENUM$DISR == 0x1">(*($T2*) &amp;__0)</Item>
</Expand>
</Type>

<Type Name="core::ptr::non_null::NonNull&lt;*&gt;">
<DisplayString>{(void*) pointer}</DisplayString>
<Expand>
<Item Name="[value]">*pointer</Item>
</Expand>
</Type>
</AutoVisualizer>
29 changes: 29 additions & 0 deletions src/etc/natvis/libstd.natvis
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,4 +72,33 @@
</CustomListItems>
</Expand>
</Type>

<Type Name="std::ffi::c_str::CString">
<DisplayString>{inner.data_ptr,s}</DisplayString>
<Expand>
<Synthetic Name="[chars]">
<Expand>
<ArrayItems>
<Size>inner.length</Size>
<ValuePointer>(char*)inner.data_ptr</ValuePointer>
</ArrayItems>
</Expand>
</Synthetic>
</Expand>
</Type>

<Type Name="std::ffi::c_str::CStr">
<DisplayString>{(char*) inner}</DisplayString>
<Expand>
<Synthetic Name="[chars]">
<DisplayString>{(char*) inner}</DisplayString>
<Expand>
<ArrayItems>
<Size>strlen((char *) inner) + 1</Size>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this could cause an exception if inner was NULL.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe inner cannot be null as it's guaranteed to contain at least a null byte:

pub struct CStr {
    inner: [c_char],
}

impl CStr {
 pub fn from_bytes_with_nul(bytes: &[u8]) -> Result<&CStr, FromBytesWithNulError> {
        let nul_pos = memchr::memchr(0, bytes);
        if let Some(nul_pos) = nul_pos {
            if nul_pos + 1 != bytes.len() {
                return Err(FromBytesWithNulError::interior_nul(nul_pos));
            }
            Ok(unsafe { CStr::from_bytes_with_nul_unchecked(bytes) })
        } else {
            Err(FromBytesWithNulError::not_nul_terminated())
        }
    }
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation for CStr::from_ptr is less reassuring:

* There is no guarantee to the validity of ptr.
* The returned lifetime is not guaranteed to be the actual lifetime of ptr.
* There is no guarantee that the memory pointed to by ptr contains a valid nul terminator byte at the end of the string.
* It is not guaranteed that the memory pointed by ptr won't change before the CStr has been destroyed.

Note: This operation is intended to be a 0-cost cast but it is currently implemented with an up-front calculation of the length of the string. This is not guaranteed to always be the case.

So indeed, it looks safe now, but may not always be safe.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That’s the unsafe documentation of from_ptr. The ptr must meet those criteria in order for the call to from_ptr to be safe. So, users of CStr can assume that the internal pointer does meet those criteria. If it doesn’t, it’s a big somewhere else (a caller of from_ptr messed up).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debuggers are used to help track down bugs like this, not introduce exceptions where there might not have been one before, even if the debugger is helping expose a latent bug.

At any rate, I don't have a strong preference here - I just wanted to raise the issue. While it's nice to see the length of the CStr automatically, it also doesn't feel critical if there are downsides.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fair though I think the debugger showing an error saying it can't show some detail would be a hint to the user that something is very wrong. If the debugger can't show CStr characters, then the user knows there has been an error in unsafe. I actually don't see this as a downside personally.

<ValuePointer>(char*)inner</ValuePointer>
</ArrayItems>
</Expand>
</Synthetic>
</Expand>
</Type>
</AutoVisualizer>