Bug 210028 - ensureStillAliveHere can take the value in any location
Summary: ensureStillAliveHere can take the value in any location
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-04-05 08:56 PDT by Keith Miller
Modified: 2020-04-05 16:25 PDT (History)
7 users (show)

See Also:


Attachments
Patch (1.54 KB, patch)
2020-04-05 08:58 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (2.25 KB, patch)
2020-04-05 09:25 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (2.25 KB, patch)
2020-04-05 10:27 PDT, Keith Miller
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Miller 2020-04-05 08:56:17 PDT
ensureStillAliveHere can take the value in any location
Comment 1 Keith Miller 2020-04-05 08:58:41 PDT
Created attachment 395509 [details]
Patch
Comment 2 Mark Lam 2020-04-05 09:22:53 PDT
Comment on attachment 395509 [details]
Patch

r=me.  Please also fix the variant in JSValue too.
Comment 3 Keith Miller 2020-04-05 09:25:24 PDT
Created attachment 395511 [details]
Patch for landing
Comment 4 Keith Miller 2020-04-05 09:25:49 PDT
(In reply to Mark Lam from comment #2)
> Comment on attachment 395509 [details]
> Patch
> 
> r=me.  Please also fix the variant in JSValue too.

Ah, I missed that one. Done.
Comment 5 Mark Lam 2020-04-05 09:31:56 PDT
Actually, I think the “clobbers memory” declaration is needed.  Otherwise,the compiler can optimize the whole asm statement away.

https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html
“Note, however, that if the code that follows the asm statement makes no use of any of the output operands, the GCC optimizers may discard the asm statement as unneeded (see Volatile).”

I think :memory takes care of this.
Comment 6 Keith Miller 2020-04-05 09:34:07 PDT
(In reply to Mark Lam from comment #5)
> Actually, I think the “clobbers memory” declaration is needed. 
> Otherwise,the compiler can optimize the whole asm statement away.
> 
> https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html
> “Note, however, that if the code that follows the asm statement makes no use
> of any of the output operands, the GCC optimizers may discard the asm
> statement as unneeded (see Volatile).”
> 
> I think :memory takes care of this.

That's what the volatile does.
https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Volatile
GCC’s optimizers sometimes discard asm statements if they determine there is no need for the output variables. Also, the optimizers may move code out of loops if they believe that the code will always return the same result (i.e. none of its input values change between calls). Using the volatile qualifier disables these optimizations.
Comment 7 Mark Lam 2020-04-05 09:35:39 PDT
Comment on attachment 395511 [details]
Patch for landing

Nevermind.  https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Volatile

“ Using the volatile qualifier disables these optimizations. asm statements that have no output operands, including asm goto statements, are implicitly volatile.”
Comment 8 EWS 2020-04-05 09:57:23 PDT
Patch 395511 does not build
Comment 9 Keith Miller 2020-04-05 10:27:54 PDT
Created attachment 395516 [details]
Patch for landing
Comment 10 EWS 2020-04-05 11:23:32 PDT
Committed r259554: <https://trac.webkit.org/changeset/259554>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395516 [details].
Comment 11 Radar WebKit Bug Importer 2020-04-05 11:24:12 PDT
<rdar://problem/61318951>
Comment 12 Yusuke Suzuki 2020-04-05 16:08:23 PDT
I think this is not correct. This needs to be a compiler-fence and "memory" is used to make it so.
e.g. WTF::compilerFence.

241 // Just a compiler fence. Has no effect on the hardware, but tells the compiler
242 // not to move things around this call. Should not affect the compiler's ability
243 // to do things like register allocation and code motion over pure operations.
244 inline void compilerFence()
245 {
246 #if OS(WINDOWS) && !COMPILER(GCC_COMPATIBLE)
247     _ReadWriteBarrier();
248 #else
249     asm volatile("" ::: "memory");
250 #endif
251 }
Comment 13 Yusuke Suzuki 2020-04-05 16:25:24 PDT
Committed r259558: <https://trac.webkit.org/changeset/259558>