mozilla :: #ionmonkey

13 Sep 2017
01:58dmajorAny JIT experts around at this hour?
02:00tcampbellkind of around
02:01tcampbelldmajor: whats up?
02:01dmajortcampbell: do you know what would generate code like this?
02:01dmajorI have a build (with no debug symbols :( ) where we're calling this code with a "normal" esp, so it hits the int 3
02:03tcampbelluhh... no idea.. it looks like it is testing low tag bits
02:07tcampbelldmajor: might be JSString decoding
02:07dmajortcampbell: is it normal for JIT code to re-purpose the esp register/
02:08tcampbellis that esp or *esp. (I always confuse myself on intel vs att)
02:10dmajorthe register itself, no deref
02:11tcampbellmy understanding is Ion / Baseline won't repurpose. Not sure what WASM does
02:12dmajorhmm, I'd be surprised if this was wasm. this is just startup of firefox on a fresh profile
02:13tcampbelllet me poke in searchfox and see if anything comes to mind
02:14tcampbelldmajor: this must be it
02:15tcampbellalign stack with offset
02:16dmajortcampbell: ooh, that looks promising. thanks!
02:16tcampbelldmajor: looks like the callers with non-zero offset are mainly trampolines
02:20dmajortcampbell: my esp in this case is 0x0065a700 -- that looks good for just about any alignment. why is it considered bad?
02:20tcampbelldmajor: the function is computing for alignment after a small value is pushed
02:20* dmajor doesn't know what that means
02:21tcampbellsorry. It wants to check alignment, but doesn't have the final esp value yet
02:21tcampbellcheck_alignment(esp - 4)
02:22tcampbellso that after it pushes a value to prep for a call, it will be happily aligned
02:22dmajoroh, so it wants the raw value of esp to be _un_aligned, so that a `push` will align it?
02:22tcampbellthat is a better way to explain
02:22dmajorhmmm. I have no idea what that means for my specific case.
02:23tcampbelldmajor: pretty sure it is
02:23dmajorthis code is being called from a function pointer in libxul, so the raw esp _is_ aligned
02:23dmajorit's a 32-bit build
02:24dmajorah, ok
02:26dmajortcampbell: can you help me translate that into english? does that want the stack pointer to be 4-byte aligned?
02:26dmajoroh wait
02:26tcampbell16-byte alligned after pushing a 4-byte value
02:27dmajoris that realistic to expect though?
02:27dmajorif the caller is c++, couldn't it have any 4-byte aligned value?
02:28tcampbellno, something is using the wrong entry point
02:28dmajorwhat does "entry point" mean in this context?
02:29tcampbelltrampoline to IonMonkey
02:29* tcampbell is still looking up specifics
02:31tcampbellgive me a few minutes to grab a snack and I can help trace this out
02:33dmajorok, thanks
02:35dmajortcampbell: this may be relevant
02:35dmajoron 32-bit, like I suspected, stack can normally be any 4-byte aligned value
02:35dmajorbut what I haven't told you is that this is a mingw build! so we're taking the GNUC path
02:38tcampbellare mingw builds supposed to work?
02:40dmajorwe're attempting to stand them up in CI so that we don't keep breaking the tor people all the time
02:41tcampbellah.. so we very well could have parameterization issues
02:41dmajorso, in general, yes
02:42dmajorwhat's a parameterization issue?
02:42tcampbell*wrong macro values for the windows but not msvc
02:43dmajorI wonder, since this code is ifdef DEBUG, would anything horrible happen if we ran an opt build without these checks?
02:46tcampbellthat sounds like a bad idea
02:48dmajorhaha ok :)
02:50tcampbelldmajor: I wonder is mingw doesn't provide the 16b alignment that __GNUC__ does
02:50dmajortcampbell: yes, I'm wondering the same thing myself
02:50tcampbellwhich wouldn't be surprising since they can sort of link with msvc
02:50dmajorI think I have enough information to take this back to the mingw experts
02:50dmajorthanks for the help!
02:55tcampbelldmajor: see also
02:56tcampbelllooks like IonMonkey shouldn't care, but WASM might. Either way, Luke might have suggestions for appropriate fixes
02:56dmajorso before that change, we had a comment saying "GCC does foo" and requiring all compilers to do foo? that's a little frightening :)
02:57tcampbelldmajor: seems we didn't use it before for nearly anything. According to that back, it was noticed when asserting the C++ -> WASM edges
02:58tcampbellI suspect the answer is just add a mingw case to the ifdef
02:58dmajorI wonder if that's still true in today's tree
02:58dmajoryeah me too
02:58dmajorbut I will let someone who knows more about the situation than me post the patch :)
03:00tcampbellsounds good. I don't know even of mingw or wasm to be confident
03:00tcampbell*enough of
03:05tcampbelldmajor: at the very least, changing that macro, you should be able to continue testing
03:07dmajoryeah hopefully
08:12jandemnbp: will resurrect my callWithABI testing patch today, will be nice to have in 57
09:34nbpjandem: Cool, I will try to review it ASAP, such that we can have it in 57 then.
09:34jandemnbp: cool
12:30nbpjandem: ping me, as soon as you submit the patch. ;)
13:11jandemnbp: sure, mostly done
13:12nbpjandem, sstangl, tcampbell, Navid, arai, evilpie: Jit meeting in less than 48 minutes.
13:34jandemnbp: posted
13:35nbpjandem: I will do that after the Jit meeting and my 1:1 with naveed.
13:38jandemnbp: sure
14:03nbpstand-ups starting in a few seconds
14:09nbptcampbell: are you in the room?
14:09tcampbellnbp: yeah, vid on
14:09nbptcampbell: I can see you, unless you are the unnammed person
14:10tcampbellnbp: I just named myself
14:13tcampbellI wonder if boxing changes would be more noticeable on mobile
14:17nbpevilpie: write it down ;)
14:17* tcampbell shakes fist at WebRTC
14:17evilpienbp: no idea, I think it keeps using the wrong mic
14:17evilpieanyway have been reviewing some patches
14:17evilpieI am looking optimizing some builtins, but I am not sure which
14:18evilpiethat's it
14:18evilpieyeah I keep looking at the list
14:45evilpiesorry have to go
14:45tcampbellthanks for stopping in
14:49jandemtcampbell: on 32-bit our boxing format is pretty great so i doubt it would help arm32 much - could help arm644
14:50tcampbellmakes sense
16:00nbpjandem: isn't the Jit Runtime already ThreadLocal?
16:21jandemnbp: it's more main thread only. If this is about my patch, we want to use jscontext there because compilation thread calls some of these functions
16:21jandemand these threads have their one context
16:22jandemstupid auto correct
16:22nbpjandem: ok.
16:22nbpjandem: and yes this was about the patch.
16:34nbpjandem: r+
17:13bbouvierjandem, ping?
17:15bbouvierjandem you suggested in bug 1360211 to rename JitActivation::isWasm() to JitActivation::hasWasmExitFP(), which makes sense, because it describes better what it does. So luke suggested to have Activation::isWasm(), but that would also be a lie for the same reason you mentioned; what do you think of the following names instead: Activation::isWasmJit(
17:15bbouvier), Activation::isJitWithWasmExitFP() (kinda too verbose), Activation::isJitWasm()? (i prefer the first one, isWasmJit(), but having a second opinion would prevent later renamings)
17:21jandembbouvier: I think isWasmJit introduces the same confusion as isWasm, so I'd prefer the more verbose one
17:21jandembbouvier: are there many calls to this? I thought usually we'd know we have a jit activation
17:22bbouvierjandem, but now a Jit activation can be a wasm or non-wasm (JS) activation
17:22bbouvierso isWasmJit == isJit() and asJit()->hasWasmExitFP()
17:22jandemyeah I know, but I mean don't we know isJit() == true statically in most cases?
17:24jandembbouvier: another option is to just have hasWasmExitFP on Activation too...
17:25bbouvierjandem, isJit() == true statically in most cases? => yes, but the goal of the shorthand was to have a single function that does both checks
17:25bbouvierwould Activation::hasWasmExitFP() short-circuit if !isJit()?
17:26jandemyeah just isJit && asJit->has...
17:27bbouvieryeah, that's even a simpler name, i like it
17:27bbouvierjandem, cheers
17:28jandembbouvier: np! cool to see everything falling into place now
14 Sep 2017
No messages
Last message: 8 days and 17 minutes ago