1612921 - Add Gecko_SetLengthCString and Gecko_FinalizeCString to the prefix list

Status: RESOLVED FIXED

(Socorro :: Signature, task)

Description

[Andrew McCreight [:mccr8]]

I guess some compiler change or whatever caused this to start showing up in signatures.

I looked through the list of signatures that contain Gecko_, and found Gecko_FinalizeCString. Gecko_SetLengthString is only showing up on extremely old builds so I won't add it.

Gecko_SetLengthCString bp-e08fb9b2-3af5-4265-b9bd-04fea0200131
Gecko_FinalizeCString bp-9804c04c-a25e-4a54-85d0-e61150200202

Comment 1

[Andrew McCreight [:mccr8]]

https://github.com/mozilla-services/socorro/pull/5206

Comment 2

[Nathan Froyd [:froydnj]]

(In reply to Andrew McCreight [:mccr8] from comment #0)

I guess some compiler change or whatever caused this to start showing up in signatures.

I looked through the list of signatures that contain Gecko_, and found Gecko_FinalizeCString. Gecko_SetLengthString is only showing up on extremely old builds so I won't add it.

Gecko_SetLengthCString bp-e08fb9b2-3af5-4265-b9bd-04fea0200131
Gecko_FinalizeCString bp-9804c04c-a25e-4a54-85d0-e61150200202

This is sort of worrying, because I would expect these sort of glue functions to be inlined via cross-language LTO. At least the Gecko_FinalizeCString change is in a build that should have all the fancy Rust LTO bits turned on.

Comment 3

[Andrew McCreight [:mccr8]]

In at least those two crashes I linked, it looks like it is being called from C++.

However, the most common signature, Gecko_ReleaseAtom, and another common signature, Gecko_AddRefAtom, do look like they are mostly being called from Rust in Stylo:

bp-32d9c57f-2be6-4c94-b84c-db97b0200203
bp-d0f80096-8f5d-4078-91d7-9e1ae0200203

Comment 4

[(Away)]

(In reply to Nathan Froyd [:froydnj] from comment #2)

Gecko_SetLengthCString bp-e08fb9b2-3af5-4265-b9bd-04fea0200131
Gecko_FinalizeCString bp-9804c04c-a25e-4a54-85d0-e61150200202

This is sort of worrying, because I would expect these sort of glue functions to be inlined via cross-language LTO. At least the Gecko_FinalizeCString change is in a build that should have all the fancy Rust LTO bits turned on.

The Gecko_ function is a bit misleading here. The body of nsTSubstring::SetLength did get inlined into Gecko_SetLengthCString (yay cross-language!). And then, since Gecko_SetLengthCString didn't do any additional work but merely pass through the call, that means that Gecko_SetLengthCString and nsTSubstring::SetLength are now one and the same. URLEntry::ReadLocation was actually trying to call nsTSubstring::SetLength, but we symbolicated it as Gecko_SetLengthCString since they're the same function (the source link in Socorro still retains the nsTSubstring.cpp filename).

Same goes for the one with Gecko_FinalizeCString.

So I guess the question is really, why did we inline only one level instead of two? By eyeball, the whole thing could have been inlined into URLEntry::ReadLocation. This is a wild guess, but I'm thinking maybe there were so many callers that it would have been a large size increase and the profiles didn't justify it. But anyway, at least it's not a cross-language breakdown specifically.

Comment 5

[Michael Woerister]

Yes, that case tripped me up a few times when investigating xLTO.

cpp_fn <-- wrapper_fn <-- rust_fn

I initially expected that wrapper_fn should always be inlined into rust_fn because it's small, but sometimes cpp_fn gets inlined into wrapper_fn, which in turn makes it too big to be inlined into rust_fn.

Comment 6

[Will Kahn-Greene [:willkg] ET needinfo? me]

Attachment: pr 5215: Bug 1612921 - Add some CString functions to the prefix list

Comment 7

[Will Kahn-Greene [:willkg] ET needinfo? me]

willkg merged PR #5215: "Bug 1612921 - Add some CString functions to the prefix list" in 2674fd1.

Comment 8

[Will Kahn-Greene [:willkg] ET needinfo? me]

This went to prod just now with deploy bug #1613695.