<rdar://problem/62938878>
Created attachment 399028 [details] proposed patch. Let's try this on the EWS.
Created attachment 399031 [details] proposed patch.
Comment on attachment 399031 [details] proposed patch. Ready for review.
Comment on attachment 399031 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=399031&action=review > Source/WTF/ChangeLog:3 > + Introduce WTF::Config and put Signal.cpp's init-once globals in it. WebKit coding style says we use words, not abbreviations. Why "Config"?
Comment on attachment 399031 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=399031&action=review >> Source/WTF/ChangeLog:3 >> + Introduce WTF::Config and put Signal.cpp's init-once globals in it. > > WebKit coding style says we use words, not abbreviations. Why "Config"? Config is commonly used all over our code and is well recognized to mean configuration e.g. config.h is not named configuration.h. The reason I use Config is because it contains all the effectively "constant" global values that configures the behavior of our system. The are initialized and then expected to remain unchanged by our code (hence, effectively "constant"). The current instances of these Configs are g_gigacageConfig, g_wtfConfig, and g_jscConfig. I'm open to a better name if you have one that is concise and short.
Comment on attachment 399031 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=399031&action=review >>> Source/WTF/ChangeLog:3 >>> + Introduce WTF::Config and put Signal.cpp's init-once globals in it. >> >> WebKit coding style says we use words, not abbreviations. Why "Config"? > > Config is commonly used all over our code and is well recognized to mean configuration e.g. config.h is not named configuration.h. The reason I use Config is because it contains all the effectively "constant" global values that configures the behavior of our system. The are initialized and then expected to remain unchanged by our code (hence, effectively "constant"). The current instances of these Configs are g_gigacageConfig, g_wtfConfig, and g_jscConfig. I'm open to a better name if you have one that is concise and short. Configuration is not long and it’s a normal word. The header "config.h" was not named by the WebKit project; we chose it for compatibility with autoconf. I don’t think the names g_gigacageConfig, g_wtfConfig, and g_jscConfig are consistent with WebKit coding style.
(In reply to Darin Adler from comment #6) > Comment on attachment 399031 [details] > proposed patch. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=399031&action=review > > >>> Source/WTF/ChangeLog:3 > >>> + Introduce WTF::Config and put Signal.cpp's init-once globals in it. > >> > >> WebKit coding style says we use words, not abbreviations. Why "Config"? > > > > Config is commonly used all over our code and is well recognized to mean configuration e.g. config.h is not named configuration.h. The reason I use Config is because it contains all the effectively "constant" global values that configures the behavior of our system. The are initialized and then expected to remain unchanged by our code (hence, effectively "constant"). The current instances of these Configs are g_gigacageConfig, g_wtfConfig, and g_jscConfig. I'm open to a better name if you have one that is concise and short. > > Configuration is not long and it’s a normal word. > > The header "config.h" was not named by the WebKit project; we chose it for > compatibility with autoconf. > > I don’t think the names g_gigacageConfig, g_wtfConfig, and g_jscConfig are > consistent with WebKit coding style. 1. I think of Config as a term of art. It's so commonly used and understood in software. We don't special out VirtualMachine instead of VM, DocumentObjectModel instead of DOM, etc. 2. I did not quote 3 examples to convince you of the precedence, merely to point out that it would be longer to type things like this: g_gigacageConfiguration.ensureGigacageHasBeenCalled g_gigacageConfiguration.shouldBeEnabledHasBeenCalled g_jscConfiguration.startOfFixedWritableMemoryPool It adds more typing and reading with 0 benefit in code comprehension. If anything, it slightly adds a bit of mental friction to read code because one would have to process more text. 3. Precedence. % cd Source % grep -rn Config * | grep -v Configuration | grep -v Configurable | grep -v ChangeLog | grep -v props | grep -vi make | grep -vi xcode | grep -v '.pyc' | grep -v ThirdParty | grep -v 'Scripts/' | wc 1024 5432 119346 Minus my uses (approximately): % grep -rn Config * | grep -v Configuration | grep -v Configurable | grep -v ChangeLog | grep -v props | grep -vi make | grep -vi xcode | grep -v '.pyc' | grep -v ThirdParty | grep -v 'Scripts/' | grep -v g_wtfConfig | grep -v g_gigacageConfig | grep -v g_jscConfig | grep -v WTF::Config | grep -v JSC::Config | grep -v Gigacage::Config | wc 891 4827 104559 But if you strongly object to my use of it, I'll change mine to Configuration.
(In reply to Mark Lam from comment #7) > We don't special out VirtualMachine instead of VM, DocumentObjectModel > instead of DOM, etc. typo: /special/spell out/
(In reply to Mark Lam from comment #7) > I think of Config as a term of art. Yes, and I also think most of the programming world heavily uses abbreviations. Our approach of using words is atypical. > But if you strongly object to my use of it, I'll change mine to > Configuration. I do. But since I am not the sole decider here please don’t change it based only on my opinion.
Comment on attachment 399031 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=399031&action=review r=me with comment. >>>>> Source/WTF/ChangeLog:3 >>>>> + Introduce WTF::Config and put Signal.cpp's init-once globals in it. >>>> >>>> WebKit coding style says we use words, not abbreviations. Why "Config"? >>> >>> Config is commonly used all over our code and is well recognized to mean configuration e.g. config.h is not named configuration.h. The reason I use Config is because it contains all the effectively "constant" global values that configures the behavior of our system. The are initialized and then expected to remain unchanged by our code (hence, effectively "constant"). The current instances of these Configs are g_gigacageConfig, g_wtfConfig, and g_jscConfig. I'm open to a better name if you have one that is concise and short. >> >> Configuration is not long and it’s a normal word. >> >> The header "config.h" was not named by the WebKit project; we chose it for compatibility with autoconf. >> >> I don’t think the names g_gigacageConfig, g_wtfConfig, and g_jscConfig are consistent with WebKit coding style. > > 1. I think of Config as a term of art. It's so commonly used and understood in software. > We don't special out VirtualMachine instead of VM, DocumentObjectModel instead of DOM, etc. > > 2. I did not quote 3 examples to convince you of the precedence, merely to point out that it would be longer to type things like this: > > g_gigacageConfiguration.ensureGigacageHasBeenCalled > g_gigacageConfiguration.shouldBeEnabledHasBeenCalled > g_jscConfiguration.startOfFixedWritableMemoryPool > > It adds more typing and reading with 0 benefit in code comprehension. If anything, it slightly adds a bit of mental friction to read code because one would have to process more text. > > 3. Precedence. > > % cd Source > % grep -rn Config * | grep -v Configuration | grep -v Configurable | grep -v ChangeLog | grep -v props | grep -vi make | grep -vi xcode | grep -v '.pyc' | grep -v ThirdParty | grep -v 'Scripts/' | wc > 1024 5432 119346 > > Minus my uses (approximately): > % grep -rn Config * | grep -v Configuration | grep -v Configurable | grep -v ChangeLog | grep -v props | grep -vi make | grep -vi xcode | grep -v '.pyc' | grep -v ThirdParty | grep -v 'Scripts/' | grep -v g_wtfConfig | grep -v g_gigacageConfig | grep -v g_jscConfig | grep -v WTF::Config | grep -v JSC::Config | grep -v Gigacage::Config | wc > 891 4827 104559 > > But if you strongly object to my use of it, I'll change mine to Configuration. For what it's worth, Config is pretty widely used but it Configuration is more common. I don't really care either way though. $ ag -i "configuration" Source --ignore Source/ThirdParty --ignore "*ChangeLog*" --stats-only 7861 matches 604 files contained matches The [^xc] is to skip xcconfig files. If you count them then I guess config is more common. $ ag -i "[^xc]config[^u|^\.]" Source --ignore Source/ThirdParty --ignore "*ChangeLog*" --stats-only 1900 matches 363 files contained matches > Source/WTF/wtf/threads/Signals.cpp:326 > - dataLogLn("We somehow got called for an unknown signal ", sig, ", halp."); > + dataLogLn("We somehow got called for an unknown signal ", sig, ", help."); This spelling was intentional :P > Source/WTF/wtf/threads/Signals.h:93 > + SignalHandlerMemory* alloc(Signal); This is bizarre. I think this should be: add(Signal, SignalHandler&&);
Comment on attachment 399031 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=399031&action=review r=me too with some comments > Source/JavaScriptCore/ChangeLog:9 > + 1. Initialize VMTraps' signals early not that we'll be freezing signals at the end not => now > Source/JavaScriptCore/runtime/InitializeThreading.cpp:108 > + WTF::storeLoadFence(); why? > Source/WTF/ChangeLog:10 > + we'll be initializing them on boot up, but should not have any reason to boot up => first VM initialization should not => do not (alternatively, just say for immutable constants) > Source/WTF/ChangeLog:15 > + operations that should only be called at boot time will not be called once the same as above, don't say "boot time" > Source/WTF/ChangeLog:19 > + statics) no moved are ones that cannot be moved because they require a no => not > Source/WTF/ChangeLog:20 > + non-trivial default constructor (once_flag), or are needs to be modifiable are needs => need > Source/WTF/ChangeLog:40 > + SignalHandlers::alloc() uses a mutex. In practice, this mutex will never be > + contended on because all signal handlers are now installed at boot time before but can you guarantee we're not holding the mutex when we're catching a signal? Otherwise, dead lock > Source/WTF/ChangeLog:44 > + deployments, this does not matter because signal handler installations will in deployments => in production builds? > Source/WTF/wtf/WTFConfig.cpp:43 > +void Config::disableFreezingForTesting() I'd get rid of this capability. > Source/WTF/wtf/WTFConfig.cpp:64 > + enum { > + AllowPermissionChangesAfterThis = false, > + DisallowPermissionChangesAfterThis = true > + }; seems like this could just be done with a local variable since you never use AllowPermissionChangesAfterThis > Source/WTF/wtf/WTFConfig.h:39 > +struct Config { To jump in on the naming debate from a different angle: I'd just pick something more specific than "Config"/"Configuration". This isn't really a Config, it's only one very specific thing. So maybe we can name it more specifically. > Source/WTF/wtf/WTFConfig.h:47 > + static void configureForTesting() why even have this? It's not called AFAICT > Source/WTF/wtf/WTFConfig.h:66 > + }; nit: static assert this union field size is ConfigSizeToProtect? > Source/WTF/wtf/WTFConfig.h:75 > + storeLoadFence(); why? compilerFence? > Source/WTF/wtf/WTFConfig.h:76 > + RELEASE_ASSERT(!g_wtfConfig.isPermanentlyFrozen); also assert in constructor? > Source/WTF/wtf/threads/Signals.cpp:67 > +SignalHandlerMemory* SignalHandlers::alloc(Signal signal) > +{ > + static Lock lock; > + auto locker = holdLock(lock); > + > + size_t signalIndex = static_cast<size_t>(signal); > + RELEASE_ASSERT(numberOfHandlers[signalIndex] < maxNumberOfHandlers); > + size_t nextFree = numberOfHandlers[signalIndex]++; > + return &handlers[signalIndex][nextFree]; > +} I think the safest thing to do here is just to pass in the handler too, and then: call constructor w/ the new memory storeStoreFence bump numberOfHandlers count The main reasoning here is just to make sure the code below in forEachHandler doesn't blow up if it's called at an unexpected time in startup code. Also, should we assertNotFrozen scope here? > Source/WTF/wtf/threads/Signals.cpp:70 > +inline void SignalHandlers::forEachHandler(Signal signal, const Func& func) const I wonder if we should just early return if we haven't frozen the config? That proves we got here before we expected to > Source/WTF/wtf/threads/Signals.cpp:73 > + size_t handlerIndex = numberOfHandlers[signalIndex]; to be super duper correct, with my proposed threading policy, you need a loadLoadFence or a dependency after this. You don't want the compiler/CPU loading handler before this. > Source/WTF/wtf/threads/Signals.cpp:76 > + const SignalHandler& handler = *reinterpret_cast<SignalHandler*>(memory); nit: bitwise_cast nit: this line and above line can easily be joined into the same line of code > Source/WTF/wtf/threads/Signals.cpp:213 > void handleSignalsWithMach() assert not frozen scope? > Source/WTF/wtf/threads/Signals.cpp:326 > + dataLogLn("We somehow got called for an unknown signal ", sig, ", help."); I think Keith wrote the previous way on purpose ;-) > Source/WTF/wtf/threads/Signals.cpp:379 > + // Ensure that we've initialized this before freeze the Config. before freeze => before we freeze You should say why. Currently, code is saying the same thing as a comment > Source/WTF/wtf/threads/Signals.h:113 > +static_assert(SignalHandlers::numberOfSignals < std::numeric_limits<uint8_t>::max()); this should go in the struct AFAICT
Thanks for the reviews. (In reply to Keith Miller from comment #10) > For what it's worth, Config is pretty widely used but it Configuration is > more common. I don't really care either way though. I'm just going to stick with Config for now. We can rename these (and others) to Configuration in a separate refactoring patch if desired. I still think Config is a better choice here. > > Source/WTF/wtf/threads/Signals.cpp:326 > > - dataLogLn("We somehow got called for an unknown signal ", sig, ", halp."); > This spelling was intentional :P I'm going to un-cute it. > > Source/WTF/wtf/threads/Signals.h:93 > > + SignalHandlerMemory* alloc(Signal); > > This is bizarre. I think this should be: add(Signal, SignalHandler&&); Saam talked to me about the same thing. Fixed. (In reply to Saam Barati from comment #11) > > Source/JavaScriptCore/ChangeLog:9 > > + 1. Initialize VMTraps' signals early not that we'll be freezing signals at the end > > not => now Fixed. > > Source/JavaScriptCore/runtime/InitializeThreading.cpp:108 > > + WTF::storeLoadFence(); > > why? Fixed by replacing with a compilerFence(). > > Source/WTF/ChangeLog:10 > > + we'll be initializing them on boot up, but should not have any reason to > > boot up => first VM initialization Fixed. > should not => do not > (alternatively, just say for immutable constants) Fixed. > > Source/WTF/ChangeLog:15 > > + operations that should only be called at boot time will not be called once the > > same as above, don't say "boot time" Fixed. I replaced it with initialization time. > > Source/WTF/ChangeLog:19 > > + statics) no moved are ones that cannot be moved because they require a > > no => not Fixed. > > Source/WTF/ChangeLog:20 > > + non-trivial default constructor (once_flag), or are needs to be modifiable > > are needs => need Fixed. > > Source/WTF/ChangeLog:40 > > + SignalHandlers::alloc() uses a mutex. In practice, this mutex will never be > > + contended on because all signal handlers are now installed at boot time before > > but can you guarantee we're not holding the mutex when we're catching a > signal? Otherwise, dead lock The signal catcher does not acquire the lock, and does not call alloc(). It only relies on the numberOfHandles value for that type of Signal. Per our offline discussion, I'll change the SignalHandlers::alloc() function into an add() function, and just WTFMove the handler into it. I'll also defer incrementing numberOfHandlers[signalIndex] until after the handler has been full initialized. This will allow the signal catcher side (via forEachHandler()) to be able to continue to iterate the added handlers without acquiring a lock. > > Source/WTF/ChangeLog:44 > > + deployments, this does not matter because signal handler installations will > > in deployments => in production builds? This is not quite correct. The issue isn't about what's built in. It's about what configuration is deployed in production. I'll replace "deployment" with "production configurations". > > Source/WTF/wtf/WTFConfig.cpp:43 > > +void Config::disableFreezingForTesting() > > I'd get rid of this capability. Since currently, freezing is tied to JSC's Config freezing, which, in turn, would be circumvented if g_jscConfig.disabledFreezingForTesting is set. So, this WTF specific flag is redundant. I'll remove it until we see a need to have one of its own. > > Source/WTF/wtf/WTFConfig.cpp:64 > > + enum { > > + AllowPermissionChangesAfterThis = false, > > + DisallowPermissionChangesAfterThis = true > > + }; > > seems like this could just be done with a local variable since you never use > AllowPermissionChangesAfterThis I'm going to keep this because I think it documents the values better. Note that "Allow" is false, and "Disallow" is true. Using a boolean local can get confusing. > > Source/WTF/wtf/WTFConfig.h:39 > > +struct Config { > > To jump in on the naming debate from a different angle: I'd just pick > something more specific than "Config"/"Configuration". This isn't really a > Config, it's only one very specific thing. So maybe we can name it more > specifically. Signals.cpp will not be the only client of this mechanism (more to come). So, I think Config / Configuration (or something that describes "collection of stuff that configures how the system behaves") is appropriate. Alternatively, if there's a concise name to mean "place to hold stuff we want to freeze", I think that may be appropriate too. For now, I'll keep it as is. We can play the renaming game later. > > Source/WTF/wtf/WTFConfig.h:47 > > + static void configureForTesting() > > why even have this? It's not called AFAICT Fixed: removed for same reason as disableFreezingForTesting() above. > > Source/WTF/wtf/WTFConfig.h:66 > > + }; > > nit: static assert this union field size is ConfigSizeToProtect? It's not the union size that matters. It's the size of the Config struct itself. And I already static_assert its size just below this. > > Source/WTF/wtf/WTFConfig.h:75 > > + storeLoadFence(); > > why? > > compilerFence? Yes, this should be a compilerFence. Fixed. > > Source/WTF/wtf/WTFConfig.h:76 > > + RELEASE_ASSERT(!g_wtfConfig.isPermanentlyFrozen); > > also assert in constructor? Strictly speaking, asserting in the destructor is all that is needed, and the required case. The only value in asserting in the constructor is that crash logs will be more informative. Otherwise, we may crash while trying to execute any of the other code in the client function before we get to the destructor. Since, this assertion scope is not used in any hot critical paths, I'll add assertion in the constructor as well. > > Source/WTF/wtf/threads/Signals.cpp:67 > > +SignalHandlerMemory* SignalHandlers::alloc(Signal signal) > > +{ > > + static Lock lock; > > + auto locker = holdLock(lock); > > + > > + size_t signalIndex = static_cast<size_t>(signal); > > + RELEASE_ASSERT(numberOfHandlers[signalIndex] < maxNumberOfHandlers); > > + size_t nextFree = numberOfHandlers[signalIndex]++; > > + return &handlers[signalIndex][nextFree]; > > +} > > I think the safest thing to do here is just to pass in the handler too, and > then: > call constructor w/ the new memory > storeStoreFence > bump numberOfHandlers count Fixed. > The main reasoning here is just to make sure the code below in > forEachHandler doesn't blow up if it's called at an unexpected time in > startup code. > > Also, should we assertNotFrozen scope here? My reason for not including in the first place is because, if already frozen, the WTFMove to the handler, and the bumping of the numberOfHandlers count will already trigger a crash. I'll add it because this is not a hot critical path and a crash on that assert would point more clearly to what the bug is. > > Source/WTF/wtf/threads/Signals.cpp:70 > > +inline void SignalHandlers::forEachHandler(Signal signal, const Func& func) const > > I wonder if we should just early return if we haven't frozen the config? > That proves we got here before we expected to I can do that if I hadn't gotten rid of the disabledFreezingForTesting flag. On debug/test configurations, it should be fine to proceed without the Config being frozen. The check would be: if (!g_wtfConfig.isPermanentlyFrozen && g_wtfConfig.disabledFreezingForTesting) return; I think it's fine to leave this as is. If the client-side handler has been installed, it should be able to do its work. Otherwise, the client-side handler should be doing its own check for a "not yet ready" condition. > > Source/WTF/wtf/threads/Signals.cpp:73 > > + size_t handlerIndex = numberOfHandlers[signalIndex]; > > to be super duper correct, with my proposed threading policy, you need a > loadLoadFence or a dependency after this. You don't want the compiler/CPU > loading handler before this. Thanks. I added a loadLoadFence(). > > Source/WTF/wtf/threads/Signals.cpp:76 > > + const SignalHandler& handler = *reinterpret_cast<SignalHandler*>(memory); > > nit: bitwise_cast It's a pointer to pointer conversion. I think it's overkill, but sure, I'll use bitwise_cast. > nit: this line and above line can easily be joined into the same line of code > > > Source/WTF/wtf/threads/Signals.cpp:213 > > void handleSignalsWithMach() > > assert not frozen scope? > > > Source/WTF/wtf/threads/Signals.cpp:326 > > + dataLogLn("We somehow got called for an unknown signal ", sig, ", help."); > > I think Keith wrote the previous way on purpose ;-) I'm still going to un-cute it. > > Source/WTF/wtf/threads/Signals.cpp:379 > > + // Ensure that we've initialized this before freeze the Config. > > before freeze => before we freeze > > You should say why. Currently, code is saying the same thing as a comment I replaced this with a comment explaining why adapted from the ChangeLog. > > Source/WTF/wtf/threads/Signals.h:113 > > +static_assert(SignalHandlers::numberOfSignals < std::numeric_limits<uint8_t>::max()); > > this should go in the struct AFAICT Fixed.
Created attachment 399074 [details] patch for landing.
Landed in r261538: <http://trac.webkit.org/r261538>.
GTK API tests are failing after this patch https://bugs.webkit.org/show_bug.cgi?id=211773