Closed Bug 1477013 Opened 7 years ago Closed 7 years ago

c++ generic handling turning things into <T> works poorly for rust

Categories

(Socorro :: Signature, task, P1)

Product:
Socorro
Component:
Signature
Type:
task

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: willkg, Assigned: willkg)

References

Details

Attachments

(1 file)

pr 4561: fix bug 1477013: fix signature generation for rust trait methods
53 bytes, text/x-github-pull-request
Details | Review
Paraphrasing Ted on IRC, Socorro's signature generation transforms things like <Foo> into <T> which works nicely for C++ generic types. However, it works poorly for Rust where the types are important and we lose a lot of information when converting them to <T>. For example: bp-0ca773e7-2b08-4d1a-8fd7-67b4e0180713 gets this signature (after #1475820 lands): <T>::execute That should be: <rayon_core::job::HeapJob<BODY> as rayon_core::job::Job>::execute This bug covers fixing this.
Making this a P1 and grabbing it to do soon.
Assignee: nobody → willkg
Status: NEW → ASSIGNED
Priority: -- → P1
Ted said he wanted to add stuff to this. Adding a needinfo so it's on his todo list.
Flags: needinfo?(ted)
CCing a few people that know about Rust type signatures and crash reporting and whatnot. We currently have a signature transformation in Socorro that turns template parameters into just `T`, mostly so that heavily-used templates that get code-folded by linkers don't result in confusing signatures that change arbitrarily (the canonical example here would be nsCOMPtr<T>). This doesn't work great for Rust function signatures in all cases, since trait methods just wind up being `<T>::method`. This shouldn't be hard to fix but I don't know what would be the most desirable outcome. I can think of two options: 1) Just don't do this transformation if the literal string ' as ' is in the contents of the <>. We'd preserve signatures like `<rayon_core::job::HeapJob<BODY> as rayon_core::job::Job>::execute` exactly as-is. 2) If the literal string ' as ' is in the contents of the <>, replace the whole thing with either the concrete type name or the trait name, so we'd turn that signature into either `<rayon_core::job::HeapJob<BODY>>::execute` or `<rayon_core::job::Job>::execute`. Do any of you folks have a suggestion for what'd be most useful here? As with all parts of signature generation, the primary motivations are ensuring that similar crashes wind up with the same signature, different crashes wind up with different signatures, and crash signatures don't change between builds if the code involve hasn't changed.
Flags: needinfo?(ted)
For the symbol there I'd probably recommend listing the specific type like `<rayon_core::job::Job>::execute`. The only ambiguity there is if multiple traits have the same method name, but you can often from the context of the stack trace always figure out the right trait. Otherwise `<Trait>::name` doesn't tell you anything about the specific implementation to look into!
Option (1) sounds like a reasonable thing to try first.
Note that before bug 1474871, those symbols were not demangled at all.
(In reply to Mike Hommey [:glandium] from comment #6) > Note that before bug 1474871, those symbols were not demangled at all. Not *exactly*. Prior to bug 1309172 they would have been demangled using C++ demangling rules, which actually seems to produce the same result for this symbol: $ c++filt '_ZN77_$LT$rayon_core..job..HeapJob$LT$BODY$GT$$u20$as$u20$rayon_core..job..Job$GT$7execute17h05c037a08fbeb197E' <rayon_core::job::HeapJob<BODY> as rayon_core::job::Job>::execute Since bug 1474871 we're demangling using Rust demangling rules, which ought to produce better results overall.
Depends on: 1481282
Let's summarize a bit. Sounds like we should try "option 1" from comment #3. > Just don't do this transformation if the literal string ' as ' is in the contents of the <>. We'd preserve signatures like `<rayon_core::job::HeapJob<BODY> as rayon_core::job::Job>::execute` exactly as-is. I spent some time looking at frames from Rust source files in stacks in crash reports [1]. Here's an egregious example: static void core::ptr::drop_in_place<hashglobe::table::RawTable<style::gecko_string_cache::Atom, smallvec::SmallVec<[style::stylist::Rule; 1]>>>(struct hashglobe::table::RawTable<style::gecko_string_cache::Atom, smallvec::SmallVec<[style::stylist::Rule; 1]>>*) After implementing option 1, this frame gets normalized as: static void core::ptr::drop_in_place<hashglobe::table::RawTable<style::gecko_string_cache::Atom, smallvec::SmallVec<[style::stylist::Rule; 1]>>> That's 144 characters long. [2] Signature generation truncates signatures to 255 characters, so this one frame takes up over half of that. That's pretty long. Is that ok/good-enough-for-now? Does it suggest we should try something different? [1] Doing this by hand is tiresome and time-consuming. I'll write a script if I find myself looking at more frames. [2] Bug #1478383 covers getting rid of "static" and "void", too, which would drop this to 132 characters.
Oops--my bad. That extremely long string has no " as " in it, so it doesn't trigger the conditions we're looking at. Sorry for the noise! I'll try out option 1 and report back with how signatures change.
Commits pushed to master at https://github.com/mozilla-services/socorro https://github.com/mozilla-services/socorro/commit/734359ba2f140ff76396c6699079213cba649b09 fix bug 1477013: rewrite collapse to handle rust trait methods When signature generation normalizes frames, it takes the function and collapses it down to a more "neutral form" which creates a signature that is more likely to match other crash reports that have the "same problem". That works fine with functions from C/C++ but doesn't work well with functions from Rust where < and > has different meaning. This reworks the collapse code to simplify it a bit, adds tests, adds documentation, and adds handling for the case where the stuff between < and > is a Rust trait method. https://github.com/mozilla-services/socorro/commit/5c570393a40694d10de313639ff7686b78d837e9 Merge pull request #4561 from willkg/1477013-fix-rust-traits fix bug 1477013: fix signature generation for rust trait methods
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: