Re: [webkit-dev] Implementing Universal Second Factor (U2F)

[Rick Byers]

As I understand, there's active development going on in both chromium and
Edge for Web Authentication right now.  I'm sure those folks would love to
collaborate with someone working in WebKit (and I'm happy to make
introductions).

On Wed, Feb 22, 2017 at 6:08 PM, Sam Weinig  wrote:

>
> On Feb 22, 2017, at 1:22 PM, Ryosuke Niwa  wrote:
>
> On Wed, Feb 22, 2017 at 12:56 PM, Rick Byers  wrote:
>
>> Chrome ships with a built-in extension that exposes the high-level API
>> (which I think we all agree is a hack).  We recently had this discussion
>> 
>> about the right path forward here, and agreed that we should instead focus
>> our efforts
>> 
>> on the Web Authentication API  instead,
>> since it seemed much more likely to be something that would become
>> interoperable between browsers.
>>
>
> Boris's comment in the referenced thread makes me think that we should
> just implement https://w3c.github.io/webauthn/ if any:
>
> 1) [Gecko/Firefox] have an implementation of the FIDO U2F API behind a
>> pref so people
>> can experiment with it.
>> 2) [Gecko/Firefox] plan to ship the API being developed at
>> https://w3c.github.io/webauthn/ once it stabilizes.
>> > 3) [Gecko/Firefox] have no plans to ship the FIDO U2F API, now or in
>> the future.
>
>
> As such, I don't think we should be implementing FIDO U2F API on trunk.
>
> - R. Niwa
>
>
> Given that data, I agree with Ryosuke, and adding an implementing of the
> FIDO U2F API doesn’t seem like a great fit for WebKit.
>
> That said, Jacob, do you have any interest in working on an implementation
> of the Web Authentication specification?
>
> - Sam
>
>
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Implementing Universal Second Factor (U2F)

[Sam Weinig]

> On Feb 22, 2017, at 1:22 PM, Ryosuke Niwa  wrote:
> 
> On Wed, Feb 22, 2017 at 12:56 PM, Rick Byers  > wrote:
> Chrome ships with a built-in extension that exposes the high-level API (which 
> I think we all agree is a hack).  We recently had this discussion 
> 
>  about the right path forward here, and agreed that we should instead focus 
> our efforts 
> 
>  on the Web Authentication API  instead, 
> since it seemed much more likely to be something that would become 
> interoperable between browsers.
> 
> Boris's comment in the referenced thread makes me think that we should just 
> implement https://w3c.github.io/webauthn/  
> if any:
> 
> 1) [Gecko/Firefox] have an implementation of the FIDO U2F API behind a pref 
> so people 
> can experiment with it. 
> 2) [Gecko/Firefox] plan to ship the API being developed at 
> https://w3c.github.io/webauthn/  once it 
> stabilizes. 
> > 3) [Gecko/Firefox] have no plans to ship the FIDO U2F API, now or in the 
> > future. 
> 
> As such, I don't think we should be implementing FIDO U2F API on trunk.
> 
> - R. Niwa
> 

Given that data, I agree with Ryosuke, and adding an implementing of the FIDO 
U2F API doesn’t seem like a great fit for WebKit.

That said, Jacob, do you have any interest in working on an implementation of 
the Web Authentication specification?

- Sam
 ___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] ReadMe.md on Github

[Ryosuke Niwa]

Posted a patch on https://bugs.webkit.org/show_bug.cgi?id=168745 to
add instructions to build GTK+ port.
- R. Niwa


On Wed, Feb 22, 2017 at 2:26 PM, Ryosuke Niwa  wrote:
> On Wed, Feb 22, 2017 at 6:55 AM, Michael Catanzaro
>  wrote:
>> On Tue, 2017-02-21 at 22:53 -0800, Ryosuke Niwa wrote:
>>> I feel like GTK+ port's instruction can be consolidated enough to fit
>>> there but we might want to write some script that someone can run to
>>> do everything you need like Tools/Scripts/configure-xcode-for-ios-
>>> development for iOS first.
>>
>> Currently there are just two scripts (install-dependencies and update-
>> webkitgtk-libs), and they are sufficiently-different that they should
>> not be consolidated.
>
> Oh, I didn't realize you just had to run two scripts. We should just
> add GTK+ instruction then.
>
>> That said, a pointer from the README to somewhere else is fine for us.
>> Certainly that's the way to go if the goal is to reduce the size of the
>> README. I would go as far to remove the Mac/iOS build and run
>> instructions and just point to the website for all ports. The more
>> content we duplicate in the README, the more content we have that will
>> get out of sync with the website.
>
> Well, the point of adding ReadMe.md was to consolidate information
> needed to start hacking on WebKit without having to read dozens of web
> pages.
>
> However, in general, it's a good idea to make it as brief as possible
> so that it won't result in TL; DR.
>
> - R. Niwa
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] ReadMe.md on Github

[Ryosuke Niwa]

On Wed, Feb 22, 2017 at 6:55 AM, Michael Catanzaro
 wrote:
> On Tue, 2017-02-21 at 22:53 -0800, Ryosuke Niwa wrote:
>> I feel like GTK+ port's instruction can be consolidated enough to fit
>> there but we might want to write some script that someone can run to
>> do everything you need like Tools/Scripts/configure-xcode-for-ios-
>> development for iOS first.
>
> Currently there are just two scripts (install-dependencies and update-
> webkitgtk-libs), and they are sufficiently-different that they should
> not be consolidated.

Oh, I didn't realize you just had to run two scripts. We should just
add GTK+ instruction then.

> That said, a pointer from the README to somewhere else is fine for us.
> Certainly that's the way to go if the goal is to reduce the size of the
> README. I would go as far to remove the Mac/iOS build and run
> instructions and just point to the website for all ports. The more
> content we duplicate in the README, the more content we have that will
> get out of sync with the website.

Well, the point of adding ReadMe.md was to consolidate information
needed to start hacking on WebKit without having to read dozens of web
pages.

However, in general, it's a good idea to make it as brief as possible
so that it won't result in TL; DR.

- R. Niwa
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Why does RELEASE_ASSERT not have an error message?

[Ryosuke Niwa]

On Wed, Feb 22, 2017 at 12:41 PM, Mark Lam  wrote:
>
> I would like to point out that we might be able to get the best of both 
> worlds.  Here’s how we can do it:
>
> define RELEASE_ASSERT(assertion) do { \
> if (UNLIKELY(!(assertion))) { \
> preserveRegisterState(); \
> WTFReportAssertionFailure(__FILE__, __LINE__, WTF_PRETTY_FUNCTION, 
> #assertion); \
> restoreRegisterState(); \
> CRASH(); \
> } \
>
> preserveRegisterState() and restoreRegisterState() will carefully push and 
> pop registers onto / off the stack (like how the JIT probe works).
>

I'm afraid this would bloat the binary size too much. You can test but
I'd be surprised if this didn't negatively impact perf especially in
WebCore code.

- R. Niwa
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Implementing Universal Second Factor (U2F)

[Ryosuke Niwa]

On Wed, Feb 22, 2017 at 12:56 PM, Rick Byers  wrote:

> Chrome ships with a built-in extension that exposes the high-level API
> (which I think we all agree is a hack).  We recently had this discussion
> 
> about the right path forward here, and agreed that we should instead focus
> our efforts
> 
> on the Web Authentication API  instead,
> since it seemed much more likely to be something that would become
> interoperable between browsers.
>

Boris's comment in the referenced thread makes me think that we should just
implement https://w3c.github.io/webauthn/ if any:

1) [Gecko/Firefox] have an implementation of the FIDO U2F API behind a pref
> so people
> can experiment with it.
> 2) [Gecko/Firefox] plan to ship the API being developed at
> https://w3c.github.io/webauthn/ once it stabilizes.
> > 3) [Gecko/Firefox] have no plans to ship the FIDO U2F API, now or in
> the future.


As such, I don't think we should be implementing FIDO U2F API on trunk.

- R. Niwa
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Implementing Universal Second Factor (U2F)

[Rick Byers]

Chrome ships with a built-in extension that exposes the high-level API
(which I think we all agree is a hack).  We recently had this discussion

about the right path forward here, and agreed that we should instead focus
our efforts

on the Web Authentication API  instead,
since it seemed much more likely to be something that would become
interoperable between browsers.



On Wed, Feb 22, 2017 at 3:46 PM, Sam Weinig  wrote:

>
> On Feb 22, 2017, at 5:52 AM, Jacob Greenfield  wrote:
>
> I’m working on adding support to WebKit for FIDO U2F (JS API:
> https://fidoalliance.org/specs/fido-u2f-v1.1-id-
> 20160915/fido-u2f-javascript-api-v1.1-id-20160915.html Architecture
> overview: https://fidoalliance.org/specs/fido-u2f-v1.1-id-
> 20160915/fido-u2f-overview-v1.1-id-20160915.html ). The FIDO U2F
> specification allows a secure second factor to be used during
> authentication flow, with bidirectional verification (token verifies
> server, server verifies token and token’s knowledge of a specific private
> key). There are current implementations in Chrome, Opera, and Blink
> (Firefox). I’m primarily interested in bringing support to Safari, so that
> is the focus what I am currently working on.
>
>
> Hi Jacob, and welcome to WebKit.
>
> I went looking for how to use the feature in Chrome and Firefox (I assume
> you meant Gecko (Firefox), not Blink (Firefox)) I’m a little confused as to
> how this feature is exposed in the other browsers.  On the topic of the
> low-level MessagePort API, section 3 states “This specification does not
> describe how such a port is made available to RP web pages, as this is (for
> now) implementation and browser dependent” (https://fidoalliance.org/
> specs/fido-u2f-v1.1-id-20160915/fido-u2f-javascript-
> api-v1.1-id-20160915.html#api-levels).  Similarly, for the high-level
> API, it states in section 3.2, “Implementations may choose how to make such
> an API available to RP web pages. If such an API is provided,
> it should provide a namespace object u2f of the following interface" (
> https://fidoalliance.org/specs/fido-u2f-v1.1-id-
> 20160915/fido-u2f-javascript-api-v1.1-id-20160915.html#
> high-level-javascript-api).
>
> Do you have insight into how either of these APIs are exposed in other
> browsers? How do you plan on exposing them in WebKit?
>
> I should say, generally, I am concerned with APIs that leave important
> details like how the APIs are exposed to the implementation, as they lead
> to non-interoperable implementations.
>
> Thanks,
> - Sam
>
>
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev
>
>
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Why does RELEASE_ASSERT not have an error message?

[Mark Lam]

> On Feb 22, 2017, at 12:46 PM, Filip Pizlo  wrote:
> 
>> 
>> On Feb 22, 2017, at 12:41 PM, Mark Lam > > wrote:
>> 
>>> 
>>> On Feb 22, 2017, at 12:35 PM, Filip Pizlo >> > wrote:
>>> 
 
 On Feb 22, 2017, at 12:33 PM, Mark Lam > wrote:
 
 
> On Feb 22, 2017, at 12:16 PM, Filip Pizlo  > wrote:
> 
> 
>> On Feb 22, 2017, at 11:58 AM, Geoffrey Garen > > wrote:
>> 
>> I’ve lost countless hours to investigating CrashTracers that would have 
>> been easy to solve if I had access to register state.
> 
> The current RELEASE_ASSERT means that every assertion in what the 
> compiler thinks is a function (i.e. some function and everything inlined 
> into it) is coalesced into a single trap site.  I’d like to understand 
> how you use the register state if you don’t even know which assertion you 
> are at.
 
 Correction: they are not coalesced.  I was mistaken about that.  The fact 
 that we turn them into inline asm (for emitting the int3) means the 
 compiler cannot optimize it away or coalesce it.  The compiler does move 
 it to the end of the emitted code for the function though because we end 
 the CRASH() macro with __builtin_unreachable().
 
 Hence, each int3 can be correlated back to the RELEASE_ASSERT that 
 triggered it (with some extended disassembly work).
>>> 
>>> This never works for me.  I tested it locally.  LLVM will even coalesce 
>>> similar inline assembly.
>> 
>> With my proposal, I’m emitting different inline asm now after the int3 trap 
>> because I’m embedding line number and file strings.  Hence, even if the 
>> compiler is smart enough to compare inline asm code blobs, it will find them 
>> to be different, and hence, it doesn’t make sense to coalesce.
> 
> Are you claiming that LLVM does not currently now coalesce RELEASE_ASSERTS, 
> or that it will not coalesce them anymore after you make some change?

Here, I’m claiming that it will not coalesce after I make some changes.

> 
>> 
>>> 
 
> I believe that if you do want to analyze register state, then switching 
> back to calling some function that prints out diagnostic information is 
> strictly better.  Sure, you get less register state, but at least you 
> know where you crashed.  Knowing where you crashed is much more important 
> than knowing the register state, since the register state is not useful 
> if you don’t know where you crashed.
> 
 
 I would like to point out that we might be able to get the best of both 
 worlds.  Here’s how we can do it:
 
 define RELEASE_ASSERT(assertion) do { \
 if (UNLIKELY(!(assertion))) { \
 preserveRegisterState(); \
 WTFReportAssertionFailure(__FILE__, __LINE__, WTF_PRETTY_FUNCTION, 
 #assertion); \
 restoreRegisterState(); \
 CRASH(); \
 } \
 
 preserveRegisterState() and restoreRegisterState() will carefully push and 
 pop registers onto / off the stack (like how the JIT probe works).
>>> 
>>> Why not do the preserve/restore inside the WTFReport call?
>> 
>> Because I would like to preserve the register values that were used in the 
>> comparison that failed the assertion.
> 
> That doesn't change anything.  You can create a WTFFail that is written in 
> assembly and first saves all registers, and restores them prior to trapping.

A meaningful call here requires passing __FILE__, __LINE__, 
WTF_PRETTY_FUNCTION, and #assertion as arguments.  Hence, this will necessarily 
perturb register state at the call site.  The compiler is also free to load the 
reporting function into a register to make the call.  My approach of preserving 
regs before any code the compiler emits to make the call guarantees that we 
have the register immediately after the assertion compare.

Mark

> 
> -Filip
> 
> 
>> 
>> Mark
>> 
>>> 
 This allows us to get a log message on the terminal when we’re running 
 manually.
 
 In addition, we can capture some additional information about the 
 assertion site by forcing the compiler to emit code to capture the code 
 location info after the trapping instruction.  This is redundant but 
 provides an easy place to find this info (i.e. after the int3 instruction).
 
 #define WTFBreakpointTrap() do { \
 __asm__ volatile ("int3"); \
 __asm__ volatile( "" :  : "r"(__FILE__), "r"(__LINE__), 
 "r"(WTF_PRETTY_FUNCTION)); \
 } while (false)
 
 We can easily get the line number this way.  However, the line number is 
 not very useful by itself when we have inlining.  Hence, I also capture 
 the __FILE__ and 

Re: [webkit-dev] Why does RELEASE_ASSERT not have an error message?

[Filip Pizlo]

> On Feb 22, 2017, at 12:41 PM, Mark Lam  wrote:
> 
>> 
>> On Feb 22, 2017, at 12:35 PM, Filip Pizlo > > wrote:
>> 
>>> 
>>> On Feb 22, 2017, at 12:33 PM, Mark Lam >> > wrote:
>>> 
>>> 
 On Feb 22, 2017, at 12:16 PM, Filip Pizlo > wrote:
 
 
> On Feb 22, 2017, at 11:58 AM, Geoffrey Garen  > wrote:
> 
> I’ve lost countless hours to investigating CrashTracers that would have 
> been easy to solve if I had access to register state.
 
 The current RELEASE_ASSERT means that every assertion in what the compiler 
 thinks is a function (i.e. some function and everything inlined into it) 
 is coalesced into a single trap site.  I’d like to understand how you use 
 the register state if you don’t even know which assertion you are at.
>>> 
>>> Correction: they are not coalesced.  I was mistaken about that.  The fact 
>>> that we turn them into inline asm (for emitting the int3) means the 
>>> compiler cannot optimize it away or coalesce it.  The compiler does move it 
>>> to the end of the emitted code for the function though because we end the 
>>> CRASH() macro with __builtin_unreachable().
>>> 
>>> Hence, each int3 can be correlated back to the RELEASE_ASSERT that 
>>> triggered it (with some extended disassembly work).
>> 
>> This never works for me.  I tested it locally.  LLVM will even coalesce 
>> similar inline assembly.
> 
> With my proposal, I’m emitting different inline asm now after the int3 trap 
> because I’m embedding line number and file strings.  Hence, even if the 
> compiler is smart enough to compare inline asm code blobs, it will find them 
> to be different, and hence, it doesn’t make sense to coalesce.

Are you claiming that LLVM does not currently now coalesce RELEASE_ASSERTS, or 
that it will not coalesce them anymore after you make some change?

> 
>> 
>>> 
 I believe that if you do want to analyze register state, then switching 
 back to calling some function that prints out diagnostic information is 
 strictly better.  Sure, you get less register state, but at least you know 
 where you crashed.  Knowing where you crashed is much more important than 
 knowing the register state, since the register state is not useful if you 
 don’t know where you crashed.
 
>>> 
>>> I would like to point out that we might be able to get the best of both 
>>> worlds.  Here’s how we can do it:
>>> 
>>> define RELEASE_ASSERT(assertion) do { \
>>> if (UNLIKELY(!(assertion))) { \
>>> preserveRegisterState(); \
>>> WTFReportAssertionFailure(__FILE__, __LINE__, WTF_PRETTY_FUNCTION, 
>>> #assertion); \
>>> restoreRegisterState(); \
>>> CRASH(); \
>>> } \
>>> 
>>> preserveRegisterState() and restoreRegisterState() will carefully push and 
>>> pop registers onto / off the stack (like how the JIT probe works).
>> 
>> Why not do the preserve/restore inside the WTFReport call?
> 
> Because I would like to preserve the register values that were used in the 
> comparison that failed the assertion.

That doesn't change anything.  You can create a WTFFail that is written in 
assembly and first saves all registers, and restores them prior to trapping.

-Filip


> 
> Mark
> 
>> 
>>> This allows us to get a log message on the terminal when we’re running 
>>> manually.
>>> 
>>> In addition, we can capture some additional information about the assertion 
>>> site by forcing the compiler to emit code to capture the code location info 
>>> after the trapping instruction.  This is redundant but provides an easy 
>>> place to find this info (i.e. after the int3 instruction).
>>> 
>>> #define WTFBreakpointTrap() do { \
>>> __asm__ volatile ("int3"); \
>>> __asm__ volatile( "" :  : "r"(__FILE__), "r"(__LINE__), 
>>> "r"(WTF_PRETTY_FUNCTION)); \
>>> } while (false)
>>> 
>>> We can easily get the line number this way.  However, the line number is 
>>> not very useful by itself when we have inlining.  Hence, I also capture the 
>>> __FILE__ and WTF_PRETTY_FUNCTION.  However, I haven’t been able to figure 
>>> out how to decode those from the otool disassembler yet.
>>> 
>>> The only downside of doing this extra work is that it increases the code 
>>> size for each RELEASE_ASSERT site.  This is probably insignificant in total.
>>> 
>>> Performance-wise, it should be neutral-ish because the 
>>> __builtin_unreachable() in the CRASH() macro + the UNLIKELY() macro would 
>>> tell the compiler to put this in a slow path away from the main code path.
>>> 
>>> Any thoughts on this alternative?
>>> 
>>> Mark
>>> 
>>> 
> 
> I also want the freedom to add RELEASE_ASSERT without ruining performance 
> due to bad register allocation or making the code too large to inline. 
> For example, 

Re: [webkit-dev] Implementing Universal Second Factor (U2F)

[Sam Weinig]

> On Feb 22, 2017, at 5:52 AM, Jacob Greenfield  wrote:
> 
> I’m working on adding support to WebKit for FIDO U2F (JS API: 
> https://fidoalliance.org/specs/fido-u2f-v1.1-id-20160915/fido-u2f-javascript-api-v1.1-id-20160915.html
>  Architecture overview: 
> https://fidoalliance.org/specs/fido-u2f-v1.1-id-20160915/fido-u2f-overview-v1.1-id-20160915.html
>  ). The FIDO U2F specification allows a secure second factor to be used 
> during authentication flow, with bidirectional verification (token verifies 
> server, server verifies token and token’s knowledge of a specific private 
> key). There are current implementations in Chrome, Opera, and Blink 
> (Firefox). I’m primarily interested in bringing support to Safari, so that is 
> the focus what I am currently working on.

Hi Jacob, and welcome to WebKit.

I went looking for how to use the feature in Chrome and Firefox (I assume you 
meant Gecko (Firefox), not Blink (Firefox)) I’m a little confused as to how 
this feature is exposed in the other browsers.  On the topic of the low-level 
MessagePort API, section 3 states “This specification does not describe how 
such a port is made available to RP web pages, as this is (for now) 
implementation and browser dependent” 
(https://fidoalliance.org/specs/fido-u2f-v1.1-id-20160915/fido-u2f-javascript-api-v1.1-id-20160915.html#api-levels
 
).
  Similarly, for the high-level API, it states in section 3.2, “Implementations 
may choose how to make such an API available to RP web pages. If such an API is 
provided, it should provide a namespace object u2f of the following interface" 
(https://fidoalliance.org/specs/fido-u2f-v1.1-id-20160915/fido-u2f-javascript-api-v1.1-id-20160915.html#high-level-javascript-api
 
).

Do you have insight into how either of these APIs are exposed in other 
browsers? How do you plan on exposing them in WebKit?

I should say, generally, I am concerned with APIs that leave important details 
like how the APIs are exposed to the implementation, as they lead to 
non-interoperable implementations. 

Thanks,
- Sam

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Why does RELEASE_ASSERT not have an error message?

[Filip Pizlo]

Mark,

I know that you keep saying this.  I remember you told me this even when I was 
sitting on a RELEASE_ASSERT that had gotten rage-coalesced.

Your reasoning sounds great, but this just isn't what happens in clang.  
__builtin_trap gets coalesced, as does inline asm.

-Filip


> On Feb 22, 2017, at 12:38 PM, Mark Lam  wrote:
> 
> For some context, we used to see aggregation of the CRASH() for 
> RELEASE_ASSERT() in the old days before we switched to using the int3 trap.  
> Back then we called a crash() function that never returns.  As a result, the 
> C++ compiler was able to coalesce all the calls.  With the int3 trap emitted 
> by inline asm, the C++ compiler has less ability to determine that the crash 
> sites have the same code (probably because it doesn’t bother comparing what’s 
> in the inline asm blobs).
> 
> Mark
> 
> 
>> On Feb 22, 2017, at 12:33 PM, Mark Lam > > wrote:
>> 
>>> 
>>> On Feb 22, 2017, at 12:16 PM, Filip Pizlo >> > wrote:
>>> 
>>> 
 On Feb 22, 2017, at 11:58 AM, Geoffrey Garen > wrote:
 
 I’ve lost countless hours to investigating CrashTracers that would have 
 been easy to solve if I had access to register state.
>>> 
>>> The current RELEASE_ASSERT means that every assertion in what the compiler 
>>> thinks is a function (i.e. some function and everything inlined into it) is 
>>> coalesced into a single trap site.  I’d like to understand how you use the 
>>> register state if you don’t even know which assertion you are at.
>> 
>> Correction: they are not coalesced.  I was mistaken about that.  The fact 
>> that we turn them into inline asm (for emitting the int3) means the compiler 
>> cannot optimize it away or coalesce it.  The compiler does move it to the 
>> end of the emitted code for the function though because we end the CRASH() 
>> macro with __builtin_unreachable().
>> 
>> Hence, each int3 can be correlated back to the RELEASE_ASSERT that triggered 
>> it (with some extended disassembly work).
>> 
>>> I believe that if you do want to analyze register state, then switching 
>>> back to calling some function that prints out diagnostic information is 
>>> strictly better.  Sure, you get less register state, but at least you know 
>>> where you crashed.  Knowing where you crashed is much more important than 
>>> knowing the register state, since the register state is not useful if you 
>>> don’t know where you crashed.
>>> 
>> 
>> I would like to point out that we might be able to get the best of both 
>> worlds.  Here’s how we can do it:
>> 
>> define RELEASE_ASSERT(assertion) do { \
>> if (UNLIKELY(!(assertion))) { \
>> preserveRegisterState(); \
>> WTFReportAssertionFailure(__FILE__, __LINE__, WTF_PRETTY_FUNCTION, 
>> #assertion); \
>> restoreRegisterState(); \
>> CRASH(); \
>> } \
>> 
>> preserveRegisterState() and restoreRegisterState() will carefully push and 
>> pop registers onto / off the stack (like how the JIT probe works).
>> This allows us to get a log message on the terminal when we’re running 
>> manually.
>> 
>> In addition, we can capture some additional information about the assertion 
>> site by forcing the compiler to emit code to capture the code location info 
>> after the trapping instruction.  This is redundant but provides an easy 
>> place to find this info (i.e. after the int3 instruction).
>> 
>> #define WTFBreakpointTrap() do { \
>> __asm__ volatile ("int3"); \
>> __asm__ volatile( "" :  : "r"(__FILE__), "r"(__LINE__), 
>> "r"(WTF_PRETTY_FUNCTION)); \
>> } while (false)
>> 
>> We can easily get the line number this way.  However, the line number is not 
>> very useful by itself when we have inlining.  Hence, I also capture the 
>> __FILE__ and WTF_PRETTY_FUNCTION.  However, I haven’t been able to figure 
>> out how to decode those from the otool disassembler yet.
>> 
>> The only downside of doing this extra work is that it increases the code 
>> size for each RELEASE_ASSERT site.  This is probably insignificant in total.
>> 
>> Performance-wise, it should be neutral-ish because the 
>> __builtin_unreachable() in the CRASH() macro + the UNLIKELY() macro would 
>> tell the compiler to put this in a slow path away from the main code path.
>> 
>> Any thoughts on this alternative?
>> 
>> Mark
>> 
>> 
 
 I also want the freedom to add RELEASE_ASSERT without ruining performance 
 due to bad register allocation or making the code too large to inline. For 
 example, hot paths in WTF::Vector use RELEASE_ASSERT.
>>> 
>>> Do we have data about the performance benefits of the current 
>>> RELEASE_ASSERT implementation?
>>> 
 
 Is some compromise solution possible?
 
 Some options:
 
 (1) Add a variant of RELEASE_ASSERT that takes a string and logs.
>>> 
>>> The point of C++ 

Re: [webkit-dev] Why does RELEASE_ASSERT not have an error message?

[Mark Lam]

> On Feb 22, 2017, at 12:35 PM, Filip Pizlo  wrote:
> 
>> 
>> On Feb 22, 2017, at 12:33 PM, Mark Lam > > wrote:
>> 
>> 
>>> On Feb 22, 2017, at 12:16 PM, Filip Pizlo >> > wrote:
>>> 
>>> 
 On Feb 22, 2017, at 11:58 AM, Geoffrey Garen > wrote:
 
 I’ve lost countless hours to investigating CrashTracers that would have 
 been easy to solve if I had access to register state.
>>> 
>>> The current RELEASE_ASSERT means that every assertion in what the compiler 
>>> thinks is a function (i.e. some function and everything inlined into it) is 
>>> coalesced into a single trap site.  I’d like to understand how you use the 
>>> register state if you don’t even know which assertion you are at.
>> 
>> Correction: they are not coalesced.  I was mistaken about that.  The fact 
>> that we turn them into inline asm (for emitting the int3) means the compiler 
>> cannot optimize it away or coalesce it.  The compiler does move it to the 
>> end of the emitted code for the function though because we end the CRASH() 
>> macro with __builtin_unreachable().
>> 
>> Hence, each int3 can be correlated back to the RELEASE_ASSERT that triggered 
>> it (with some extended disassembly work).
> 
> This never works for me.  I tested it locally.  LLVM will even coalesce 
> similar inline assembly.

With my proposal, I’m emitting different inline asm now after the int3 trap 
because I’m embedding line number and file strings.  Hence, even if the 
compiler is smart enough to compare inline asm code blobs, it will find them to 
be different, and hence, it doesn’t make sense to coalesce.

> 
>> 
>>> I believe that if you do want to analyze register state, then switching 
>>> back to calling some function that prints out diagnostic information is 
>>> strictly better.  Sure, you get less register state, but at least you know 
>>> where you crashed.  Knowing where you crashed is much more important than 
>>> knowing the register state, since the register state is not useful if you 
>>> don’t know where you crashed.
>>> 
>> 
>> I would like to point out that we might be able to get the best of both 
>> worlds.  Here’s how we can do it:
>> 
>> define RELEASE_ASSERT(assertion) do { \
>> if (UNLIKELY(!(assertion))) { \
>> preserveRegisterState(); \
>> WTFReportAssertionFailure(__FILE__, __LINE__, WTF_PRETTY_FUNCTION, 
>> #assertion); \
>> restoreRegisterState(); \
>> CRASH(); \
>> } \
>> 
>> preserveRegisterState() and restoreRegisterState() will carefully push and 
>> pop registers onto / off the stack (like how the JIT probe works).
> 
> Why not do the preserve/restore inside the WTFReport call?

Because I would like to preserve the register values that were used in the 
comparison that failed the assertion.

Mark

> 
>> This allows us to get a log message on the terminal when we’re running 
>> manually.
>> 
>> In addition, we can capture some additional information about the assertion 
>> site by forcing the compiler to emit code to capture the code location info 
>> after the trapping instruction.  This is redundant but provides an easy 
>> place to find this info (i.e. after the int3 instruction).
>> 
>> #define WTFBreakpointTrap() do { \
>> __asm__ volatile ("int3"); \
>> __asm__ volatile( "" :  : "r"(__FILE__), "r"(__LINE__), 
>> "r"(WTF_PRETTY_FUNCTION)); \
>> } while (false)
>> 
>> We can easily get the line number this way.  However, the line number is not 
>> very useful by itself when we have inlining.  Hence, I also capture the 
>> __FILE__ and WTF_PRETTY_FUNCTION.  However, I haven’t been able to figure 
>> out how to decode those from the otool disassembler yet.
>> 
>> The only downside of doing this extra work is that it increases the code 
>> size for each RELEASE_ASSERT site.  This is probably insignificant in total.
>> 
>> Performance-wise, it should be neutral-ish because the 
>> __builtin_unreachable() in the CRASH() macro + the UNLIKELY() macro would 
>> tell the compiler to put this in a slow path away from the main code path.
>> 
>> Any thoughts on this alternative?
>> 
>> Mark
>> 
>> 
 
 I also want the freedom to add RELEASE_ASSERT without ruining performance 
 due to bad register allocation or making the code too large to inline. For 
 example, hot paths in WTF::Vector use RELEASE_ASSERT.
>>> 
>>> Do we have data about the performance benefits of the current 
>>> RELEASE_ASSERT implementation?
>>> 
 
 Is some compromise solution possible?
 
 Some options:
 
 (1) Add a variant of RELEASE_ASSERT that takes a string and logs.
>>> 
>>> The point of C++ assert macros is that I don’t have to add a custom string. 
>>>  I want a RELEASE_ASSERT macro that automatically stringifies the 
>>> expression and uses that as the string.
>>> 
>>> If I had a choice 

Re: [webkit-dev] Why does RELEASE_ASSERT not have an error message?

[Mark Lam]

For some context, we used to see aggregation of the CRASH() for 
RELEASE_ASSERT() in the old days before we switched to using the int3 trap.  
Back then we called a crash() function that never returns.  As a result, the 
C++ compiler was able to coalesce all the calls.  With the int3 trap emitted by 
inline asm, the C++ compiler has less ability to determine that the crash sites 
have the same code (probably because it doesn’t bother comparing what’s in the 
inline asm blobs).

Mark


> On Feb 22, 2017, at 12:33 PM, Mark Lam  wrote:
> 
>> 
>> On Feb 22, 2017, at 12:16 PM, Filip Pizlo > > wrote:
>> 
>> 
>>> On Feb 22, 2017, at 11:58 AM, Geoffrey Garen >> > wrote:
>>> 
>>> I’ve lost countless hours to investigating CrashTracers that would have 
>>> been easy to solve if I had access to register state.
>> 
>> The current RELEASE_ASSERT means that every assertion in what the compiler 
>> thinks is a function (i.e. some function and everything inlined into it) is 
>> coalesced into a single trap site.  I’d like to understand how you use the 
>> register state if you don’t even know which assertion you are at.
> 
> Correction: they are not coalesced.  I was mistaken about that.  The fact 
> that we turn them into inline asm (for emitting the int3) means the compiler 
> cannot optimize it away or coalesce it.  The compiler does move it to the end 
> of the emitted code for the function though because we end the CRASH() macro 
> with __builtin_unreachable().
> 
> Hence, each int3 can be correlated back to the RELEASE_ASSERT that triggered 
> it (with some extended disassembly work).
> 
>> I believe that if you do want to analyze register state, then switching back 
>> to calling some function that prints out diagnostic information is strictly 
>> better.  Sure, you get less register state, but at least you know where you 
>> crashed.  Knowing where you crashed is much more important than knowing the 
>> register state, since the register state is not useful if you don’t know 
>> where you crashed.
>> 
> 
> I would like to point out that we might be able to get the best of both 
> worlds.  Here’s how we can do it:
> 
> define RELEASE_ASSERT(assertion) do { \
> if (UNLIKELY(!(assertion))) { \
> preserveRegisterState(); \
> WTFReportAssertionFailure(__FILE__, __LINE__, WTF_PRETTY_FUNCTION, 
> #assertion); \
> restoreRegisterState(); \
> CRASH(); \
> } \
> 
> preserveRegisterState() and restoreRegisterState() will carefully push and 
> pop registers onto / off the stack (like how the JIT probe works).
> This allows us to get a log message on the terminal when we’re running 
> manually.
> 
> In addition, we can capture some additional information about the assertion 
> site by forcing the compiler to emit code to capture the code location info 
> after the trapping instruction.  This is redundant but provides an easy place 
> to find this info (i.e. after the int3 instruction).
> 
> #define WTFBreakpointTrap() do { \
> __asm__ volatile ("int3"); \
> __asm__ volatile( "" :  : "r"(__FILE__), "r"(__LINE__), 
> "r"(WTF_PRETTY_FUNCTION)); \
> } while (false)
> 
> We can easily get the line number this way.  However, the line number is not 
> very useful by itself when we have inlining.  Hence, I also capture the 
> __FILE__ and WTF_PRETTY_FUNCTION.  However, I haven’t been able to figure out 
> how to decode those from the otool disassembler yet.
> 
> The only downside of doing this extra work is that it increases the code size 
> for each RELEASE_ASSERT site.  This is probably insignificant in total.
> 
> Performance-wise, it should be neutral-ish because the 
> __builtin_unreachable() in the CRASH() macro + the UNLIKELY() macro would 
> tell the compiler to put this in a slow path away from the main code path.
> 
> Any thoughts on this alternative?
> 
> Mark
> 
> 
>>> 
>>> I also want the freedom to add RELEASE_ASSERT without ruining performance 
>>> due to bad register allocation or making the code too large to inline. For 
>>> example, hot paths in WTF::Vector use RELEASE_ASSERT.
>> 
>> Do we have data about the performance benefits of the current RELEASE_ASSERT 
>> implementation?
>> 
>>> 
>>> Is some compromise solution possible?
>>> 
>>> Some options:
>>> 
>>> (1) Add a variant of RELEASE_ASSERT that takes a string and logs.
>> 
>> The point of C++ assert macros is that I don’t have to add a custom string.  
>> I want a RELEASE_ASSERT macro that automatically stringifies the expression 
>> and uses that as the string.
>> 
>> If I had a choice between a RELEASE_ASSERT that can accurate report where it 
>> crashed but sometimes trashes the register state, and a RELEASE_ASSERT that 
>> always gives me the register state but cannot tell me which assert in the 
>> function it’s coming from, then I would always choose the one that can tell 
>> me where it 

Re: [webkit-dev] Why does RELEASE_ASSERT not have an error message?

[Filip Pizlo]

> On Feb 22, 2017, at 12:33 PM, Mark Lam  wrote:
> 
> 
>> On Feb 22, 2017, at 12:16 PM, Filip Pizlo > > wrote:
>> 
>> 
>>> On Feb 22, 2017, at 11:58 AM, Geoffrey Garen >> > wrote:
>>> 
>>> I’ve lost countless hours to investigating CrashTracers that would have 
>>> been easy to solve if I had access to register state.
>> 
>> The current RELEASE_ASSERT means that every assertion in what the compiler 
>> thinks is a function (i.e. some function and everything inlined into it) is 
>> coalesced into a single trap site.  I’d like to understand how you use the 
>> register state if you don’t even know which assertion you are at.
> 
> Correction: they are not coalesced.  I was mistaken about that.  The fact 
> that we turn them into inline asm (for emitting the int3) means the compiler 
> cannot optimize it away or coalesce it.  The compiler does move it to the end 
> of the emitted code for the function though because we end the CRASH() macro 
> with __builtin_unreachable().
> 
> Hence, each int3 can be correlated back to the RELEASE_ASSERT that triggered 
> it (with some extended disassembly work).

This never works for me.  I tested it locally.  LLVM will even coalesce similar 
inline assembly.

> 
>> I believe that if you do want to analyze register state, then switching back 
>> to calling some function that prints out diagnostic information is strictly 
>> better.  Sure, you get less register state, but at least you know where you 
>> crashed.  Knowing where you crashed is much more important than knowing the 
>> register state, since the register state is not useful if you don’t know 
>> where you crashed.
>> 
> 
> I would like to point out that we might be able to get the best of both 
> worlds.  Here’s how we can do it:
> 
> define RELEASE_ASSERT(assertion) do { \
> if (UNLIKELY(!(assertion))) { \
> preserveRegisterState(); \
> WTFReportAssertionFailure(__FILE__, __LINE__, WTF_PRETTY_FUNCTION, 
> #assertion); \
> restoreRegisterState(); \
> CRASH(); \
> } \
> 
> preserveRegisterState() and restoreRegisterState() will carefully push and 
> pop registers onto / off the stack (like how the JIT probe works).

Why not do the preserve/restore inside the WTFReport call?

> This allows us to get a log message on the terminal when we’re running 
> manually.
> 
> In addition, we can capture some additional information about the assertion 
> site by forcing the compiler to emit code to capture the code location info 
> after the trapping instruction.  This is redundant but provides an easy place 
> to find this info (i.e. after the int3 instruction).
> 
> #define WTFBreakpointTrap() do { \
> __asm__ volatile ("int3"); \
> __asm__ volatile( "" :  : "r"(__FILE__), "r"(__LINE__), 
> "r"(WTF_PRETTY_FUNCTION)); \
> } while (false)
> 
> We can easily get the line number this way.  However, the line number is not 
> very useful by itself when we have inlining.  Hence, I also capture the 
> __FILE__ and WTF_PRETTY_FUNCTION.  However, I haven’t been able to figure out 
> how to decode those from the otool disassembler yet.
> 
> The only downside of doing this extra work is that it increases the code size 
> for each RELEASE_ASSERT site.  This is probably insignificant in total.
> 
> Performance-wise, it should be neutral-ish because the 
> __builtin_unreachable() in the CRASH() macro + the UNLIKELY() macro would 
> tell the compiler to put this in a slow path away from the main code path.
> 
> Any thoughts on this alternative?
> 
> Mark
> 
> 
>>> 
>>> I also want the freedom to add RELEASE_ASSERT without ruining performance 
>>> due to bad register allocation or making the code too large to inline. For 
>>> example, hot paths in WTF::Vector use RELEASE_ASSERT.
>> 
>> Do we have data about the performance benefits of the current RELEASE_ASSERT 
>> implementation?
>> 
>>> 
>>> Is some compromise solution possible?
>>> 
>>> Some options:
>>> 
>>> (1) Add a variant of RELEASE_ASSERT that takes a string and logs.
>> 
>> The point of C++ assert macros is that I don’t have to add a custom string.  
>> I want a RELEASE_ASSERT macro that automatically stringifies the expression 
>> and uses that as the string.
>> 
>> If I had a choice between a RELEASE_ASSERT that can accurate report where it 
>> crashed but sometimes trashes the register state, and a RELEASE_ASSERT that 
>> always gives me the register state but cannot tell me which assert in the 
>> function it’s coming from, then I would always choose the one that can tell 
>> me where it crashed.  That’s much more important, and the register state is 
>> not useful without that information.
>> 
>>> 
>>> (2) Change RELEASE_ASSERT to do the normal debug ASSERT thing in Debug 
>>> builds. (There’s not much need to preserve register state in debug builds.)
>> 
>> That would be nice, but doesn’t make RELEASE_ASSERT 

Re: [webkit-dev] Why does RELEASE_ASSERT not have an error message?

[Mark Lam]

> On Feb 22, 2017, at 12:16 PM, Filip Pizlo  wrote:
> 
> 
>> On Feb 22, 2017, at 11:58 AM, Geoffrey Garen  wrote:
>> 
>> I’ve lost countless hours to investigating CrashTracers that would have been 
>> easy to solve if I had access to register state.
> 
> The current RELEASE_ASSERT means that every assertion in what the compiler 
> thinks is a function (i.e. some function and everything inlined into it) is 
> coalesced into a single trap site.  I’d like to understand how you use the 
> register state if you don’t even know which assertion you are at.

Correction: they are not coalesced.  I was mistaken about that.  The fact that 
we turn them into inline asm (for emitting the int3) means the compiler cannot 
optimize it away or coalesce it.  The compiler does move it to the end of the 
emitted code for the function though because we end the CRASH() macro with 
__builtin_unreachable().

Hence, each int3 can be correlated back to the RELEASE_ASSERT that triggered it 
(with some extended disassembly work).

> I believe that if you do want to analyze register state, then switching back 
> to calling some function that prints out diagnostic information is strictly 
> better.  Sure, you get less register state, but at least you know where you 
> crashed.  Knowing where you crashed is much more important than knowing the 
> register state, since the register state is not useful if you don’t know 
> where you crashed.
> 

I would like to point out that we might be able to get the best of both worlds. 
 Here’s how we can do it:

define RELEASE_ASSERT(assertion) do { \
if (UNLIKELY(!(assertion))) { \
preserveRegisterState(); \
WTFReportAssertionFailure(__FILE__, __LINE__, WTF_PRETTY_FUNCTION, 
#assertion); \
restoreRegisterState(); \
CRASH(); \
} \

preserveRegisterState() and restoreRegisterState() will carefully push and pop 
registers onto / off the stack (like how the JIT probe works).
This allows us to get a log message on the terminal when we’re running manually.

In addition, we can capture some additional information about the assertion 
site by forcing the compiler to emit code to capture the code location info 
after the trapping instruction.  This is redundant but provides an easy place 
to find this info (i.e. after the int3 instruction).

#define WTFBreakpointTrap() do { \
__asm__ volatile ("int3"); \
__asm__ volatile( "" :  : "r"(__FILE__), "r"(__LINE__), 
"r"(WTF_PRETTY_FUNCTION)); \
} while (false)

We can easily get the line number this way.  However, the line number is not 
very useful by itself when we have inlining.  Hence, I also capture the 
__FILE__ and WTF_PRETTY_FUNCTION.  However, I haven’t been able to figure out 
how to decode those from the otool disassembler yet.

The only downside of doing this extra work is that it increases the code size 
for each RELEASE_ASSERT site.  This is probably insignificant in total.

Performance-wise, it should be neutral-ish because the __builtin_unreachable() 
in the CRASH() macro + the UNLIKELY() macro would tell the compiler to put this 
in a slow path away from the main code path.

Any thoughts on this alternative?

Mark


>> 
>> I also want the freedom to add RELEASE_ASSERT without ruining performance 
>> due to bad register allocation or making the code too large to inline. For 
>> example, hot paths in WTF::Vector use RELEASE_ASSERT.
> 
> Do we have data about the performance benefits of the current RELEASE_ASSERT 
> implementation?
> 
>> 
>> Is some compromise solution possible?
>> 
>> Some options:
>> 
>> (1) Add a variant of RELEASE_ASSERT that takes a string and logs.
> 
> The point of C++ assert macros is that I don’t have to add a custom string.  
> I want a RELEASE_ASSERT macro that automatically stringifies the expression 
> and uses that as the string.
> 
> If I had a choice between a RELEASE_ASSERT that can accurate report where it 
> crashed but sometimes trashes the register state, and a RELEASE_ASSERT that 
> always gives me the register state but cannot tell me which assert in the 
> function it’s coming from, then I would always choose the one that can tell 
> me where it crashed.  That’s much more important, and the register state is 
> not useful without that information.
> 
>> 
>> (2) Change RELEASE_ASSERT to do the normal debug ASSERT thing in Debug 
>> builds. (There’s not much need to preserve register state in debug builds.)
> 
> That would be nice, but doesn’t make RELEASE_ASSERT useful for debugging 
> issues where timing is important.  I no longer use RELEASE_ASSERTS for those 
> kinds of assertions, because if I do it then I will never know where I 
> crashed.  So, I use the explicit:
> 
> if (!thing) {
>   dataLog(“…”);
>   RELEASE_ASSERT_NOT_REACHED();
> }
> 
> -Filip
> 
> 
>> 
>> Geoff
>> 
>>> On Feb 22, 2017, at 11:09 AM, Filip Pizlo  wrote:
>>> 
>>> I disagree actually.  I've lost countless hours to 

Re: [webkit-dev] Why does RELEASE_ASSERT not have an error message?

[Saam barati]

> On Feb 22, 2017, at 12:16 PM, Filip Pizlo  wrote:
> 
> 
>> On Feb 22, 2017, at 11:58 AM, Geoffrey Garen  wrote:
>> 
>> I’ve lost countless hours to investigating CrashTracers that would have been 
>> easy to solve if I had access to register state.
> 
> The current RELEASE_ASSERT means that every assertion in what the compiler 
> thinks is a function (i.e. some function and everything inlined into it) is 
> coalesced into a single trap site.  I’d like to understand how you use the 
> register state if you don’t even know which assertion you are at.
When I disassemble JavaScriptCore, I often find a succession of int3s at the 
bottom of a function. Does LLVM sometimes combine them and sometimes not?

For example, this is what the bottom of the 
__ZN3JSC20AbstractModuleRecord18getModuleNamespaceEPNS_9ExecStateE looks like:

5c25popq%r14
5c27popq%r15
5c29popq%rbp
5c2aretq
5c2bint3
5c2cint3
5c2dint3
5c2eint3
5c2fnop

- Saam
> 
> I believe that if you do want to analyze register state, then switching back 
> to calling some function that prints out diagnostic information is strictly 
> better.  Sure, you get less register state, but at least you know where you 
> crashed.  Knowing where you crashed is much more important than knowing the 
> register state, since the register state is not useful if you don’t know 
> where you crashed.
> 
>> 
>> I also want the freedom to add RELEASE_ASSERT without ruining performance 
>> due to bad register allocation or making the code too large to inline. For 
>> example, hot paths in WTF::Vector use RELEASE_ASSERT.
> 
> Do we have data about the performance benefits of the current RELEASE_ASSERT 
> implementation?
> 
>> 
>> Is some compromise solution possible?
>> 
>> Some options:
>> 
>> (1) Add a variant of RELEASE_ASSERT that takes a string and logs.
> 
> The point of C++ assert macros is that I don’t have to add a custom string.  
> I want a RELEASE_ASSERT macro that automatically stringifies the expression 
> and uses that as the string.
> 
> If I had a choice between a RELEASE_ASSERT that can accurate report where it 
> crashed but sometimes trashes the register state, and a RELEASE_ASSERT that 
> always gives me the register state but cannot tell me which assert in the 
> function it’s coming from, then I would always choose the one that can tell 
> me where it crashed.  That’s much more important, and the register state is 
> not useful without that information.
> 
>> 
>> (2) Change RELEASE_ASSERT to do the normal debug ASSERT thing in Debug 
>> builds. (There’s not much need to preserve register state in debug builds.)
> 
> That would be nice, but doesn’t make RELEASE_ASSERT useful for debugging 
> issues where timing is important.  I no longer use RELEASE_ASSERTS for those 
> kinds of assertions, because if I do it then I will never know where I 
> crashed.  So, I use the explicit:
> 
> if (!thing) {
>   dataLog(“…”);
>   RELEASE_ASSERT_NOT_REACHED();
> }
> 
> -Filip
> 
> 
>> 
>> Geoff
>> 
>>> On Feb 22, 2017, at 11:09 AM, Filip Pizlo  wrote:
>>> 
>>> I disagree actually.  I've lost countless hours to converting this:
>>> 
>>> RELEASE_ASSERT(blah)
>>> 
>>> into this:
>>> 
>>> if (!blah) {
>>> dataLog("Reason why I crashed");
>>> RELEASE_ASSERT_NOT_REACHED();
>>> }
>>> 
>>> Look in the code - you'll find lots of stuff like this.
>>> 
>>> I don't think analyzing register state at crashes is more important than 
>>> keeping our code sane.
>>> 
>>> -Filip
>>> 
>>> 
 On Feb 21, 2017, at 5:56 PM, Mark Lam  wrote:
 
 Oh yeah, I forgot about that.  I think the register state is more 
 important for crash analysis, especially if we can make sure that the 
 compiler does not aggregate the int3s.  I’ll explore alternatives.
 
> On Feb 21, 2017, at 5:54 PM, Saam barati  wrote:
> 
> I thought the main point of moving to SIGTRAP was to preserve register 
> state?
> 
> That said, there are probably places where we care more about the message 
> than the registers.
> 
> - Saam
> 
>> On Feb 21, 2017, at 5:43 PM, Mark Lam  wrote:
>> 
>> Is there a reason why RELEASE_ASSERT (and friends) does not call 
>> WTFReportAssertionFailure() to report where the assertion occur?  Is 
>> this purely to save memory?  svn blame tells me that it has been this 
>> way since the introduction of RELEASE_ASSERT in r140577 many years ago.
>> 
>> Would anyone object to adding a call to WTFReportAssertionFailure() in 
>> RELEASE_ASSERT() like we do for ASSERT()?  One of the upside 
>> (side-effect) of adding this call is that it appears to stop the 
>> compiler from aggregating all the 

Re: [webkit-dev] Why does RELEASE_ASSERT not have an error message?

[Filip Pizlo]

> On Feb 22, 2017, at 12:23 PM, Saam barati  wrote:
> 
>> 
>> On Feb 22, 2017, at 12:16 PM, Filip Pizlo > > wrote:
>> 
>> 
>>> On Feb 22, 2017, at 11:58 AM, Geoffrey Garen >> > wrote:
>>> 
>>> I’ve lost countless hours to investigating CrashTracers that would have 
>>> been easy to solve if I had access to register state.
>> 
>> The current RELEASE_ASSERT means that every assertion in what the compiler 
>> thinks is a function (i.e. some function and everything inlined into it) is 
>> coalesced into a single trap site.  I’d like to understand how you use the 
>> register state if you don’t even know which assertion you are at.
> When I disassemble JavaScriptCore, I often find a succession of int3s at the 
> bottom of a function. Does LLVM sometimes combine them and sometimes not?

Yeah.

> 
> For example, this is what the bottom of the 
> __ZN3JSC20AbstractModuleRecord18getModuleNamespaceEPNS_9ExecStateE looks like:
> 
> 5c25  popq%r14
> 5c27  popq%r15
> 5c29  popq%rbp
> 5c2a  retq
> 5c2b  int3
> 5c2c  int3
> 5c2d  int3
> 5c2e  int3
> 5c2f  nop

I’m curious how many branches target those traps.

For example in the GC, I was often getting crashes that LLVM was convinced were 
vector overflow.  Turns out that the compiler loves to coalesce other traps 
with the one from the vector overflow assert, so if you assert for some random 
reason in a function that accesses vectors, all of our tooling will report with 
total confidence that you’re overflowing a vector.

That’s way worse than not having register state.

-Filip


> 
> - Saam
>> 
>> I believe that if you do want to analyze register state, then switching back 
>> to calling some function that prints out diagnostic information is strictly 
>> better.  Sure, you get less register state, but at least you know where you 
>> crashed.  Knowing where you crashed is much more important than knowing the 
>> register state, since the register state is not useful if you don’t know 
>> where you crashed.
>> 
>>> 
>>> I also want the freedom to add RELEASE_ASSERT without ruining performance 
>>> due to bad register allocation or making the code too large to inline. For 
>>> example, hot paths in WTF::Vector use RELEASE_ASSERT.
>> 
>> Do we have data about the performance benefits of the current RELEASE_ASSERT 
>> implementation?
>> 
>>> 
>>> Is some compromise solution possible?
>>> 
>>> Some options:
>>> 
>>> (1) Add a variant of RELEASE_ASSERT that takes a string and logs.
>> 
>> The point of C++ assert macros is that I don’t have to add a custom string.  
>> I want a RELEASE_ASSERT macro that automatically stringifies the expression 
>> and uses that as the string.
>> 
>> If I had a choice between a RELEASE_ASSERT that can accurate report where it 
>> crashed but sometimes trashes the register state, and a RELEASE_ASSERT that 
>> always gives me the register state but cannot tell me which assert in the 
>> function it’s coming from, then I would always choose the one that can tell 
>> me where it crashed.  That’s much more important, and the register state is 
>> not useful without that information.
>> 
>>> 
>>> (2) Change RELEASE_ASSERT to do the normal debug ASSERT thing in Debug 
>>> builds. (There’s not much need to preserve register state in debug builds.)
>> 
>> That would be nice, but doesn’t make RELEASE_ASSERT useful for debugging 
>> issues where timing is important.  I no longer use RELEASE_ASSERTS for those 
>> kinds of assertions, because if I do it then I will never know where I 
>> crashed.  So, I use the explicit:
>> 
>> if (!thing) {
>>   dataLog(“…”);
>>   RELEASE_ASSERT_NOT_REACHED();
>> }
>> 
>> -Filip
>> 
>> 
>>> 
>>> Geoff
>>> 
 On Feb 22, 2017, at 11:09 AM, Filip Pizlo > wrote:
 
 I disagree actually.  I've lost countless hours to converting this:
 
 RELEASE_ASSERT(blah)
 
 into this:
 
 if (!blah) {
 dataLog("Reason why I crashed");
 RELEASE_ASSERT_NOT_REACHED();
 }
 
 Look in the code - you'll find lots of stuff like this.
 
 I don't think analyzing register state at crashes is more important than 
 keeping our code sane.
 
 -Filip
 
 
> On Feb 21, 2017, at 5:56 PM, Mark Lam  > wrote:
> 
> Oh yeah, I forgot about that.  I think the register state is more 
> important for crash analysis, especially if we can make sure that the 
> compiler does not aggregate the int3s.  I’ll explore alternatives.
> 
>> On Feb 21, 2017, at 5:54 PM, Saam barati > > wrote:
>> 
>> I thought the main point of moving to SIGTRAP was to 

Re: [webkit-dev] Why does RELEASE_ASSERT not have an error message?

[Filip Pizlo]

> On Feb 22, 2017, at 11:58 AM, Geoffrey Garen  wrote:
> 
> I’ve lost countless hours to investigating CrashTracers that would have been 
> easy to solve if I had access to register state.

The current RELEASE_ASSERT means that every assertion in what the compiler 
thinks is a function (i.e. some function and everything inlined into it) is 
coalesced into a single trap site.  I’d like to understand how you use the 
register state if you don’t even know which assertion you are at.

I believe that if you do want to analyze register state, then switching back to 
calling some function that prints out diagnostic information is strictly 
better.  Sure, you get less register state, but at least you know where you 
crashed.  Knowing where you crashed is much more important than knowing the 
register state, since the register state is not useful if you don’t know where 
you crashed.

> 
> I also want the freedom to add RELEASE_ASSERT without ruining performance due 
> to bad register allocation or making the code too large to inline. For 
> example, hot paths in WTF::Vector use RELEASE_ASSERT.

Do we have data about the performance benefits of the current RELEASE_ASSERT 
implementation?

> 
> Is some compromise solution possible?
> 
> Some options:
> 
> (1) Add a variant of RELEASE_ASSERT that takes a string and logs.

The point of C++ assert macros is that I don’t have to add a custom string.  I 
want a RELEASE_ASSERT macro that automatically stringifies the expression and 
uses that as the string.

If I had a choice between a RELEASE_ASSERT that can accurate report where it 
crashed but sometimes trashes the register state, and a RELEASE_ASSERT that 
always gives me the register state but cannot tell me which assert in the 
function it’s coming from, then I would always choose the one that can tell me 
where it crashed.  That’s much more important, and the register state is not 
useful without that information.

> 
> (2) Change RELEASE_ASSERT to do the normal debug ASSERT thing in Debug 
> builds. (There’s not much need to preserve register state in debug builds.)

That would be nice, but doesn’t make RELEASE_ASSERT useful for debugging issues 
where timing is important.  I no longer use RELEASE_ASSERTS for those kinds of 
assertions, because if I do it then I will never know where I crashed.  So, I 
use the explicit:

if (!thing) {
   dataLog(“…”);
   RELEASE_ASSERT_NOT_REACHED();
}

-Filip


> 
> Geoff
> 
>> On Feb 22, 2017, at 11:09 AM, Filip Pizlo  wrote:
>> 
>> I disagree actually.  I've lost countless hours to converting this:
>> 
>> RELEASE_ASSERT(blah)
>> 
>> into this:
>> 
>> if (!blah) {
>>  dataLog("Reason why I crashed");
>>  RELEASE_ASSERT_NOT_REACHED();
>> }
>> 
>> Look in the code - you'll find lots of stuff like this.
>> 
>> I don't think analyzing register state at crashes is more important than 
>> keeping our code sane.
>> 
>> -Filip
>> 
>> 
>>> On Feb 21, 2017, at 5:56 PM, Mark Lam  wrote:
>>> 
>>> Oh yeah, I forgot about that.  I think the register state is more important 
>>> for crash analysis, especially if we can make sure that the compiler does 
>>> not aggregate the int3s.  I’ll explore alternatives.
>>> 
 On Feb 21, 2017, at 5:54 PM, Saam barati  wrote:
 
 I thought the main point of moving to SIGTRAP was to preserve register 
 state?
 
 That said, there are probably places where we care more about the message 
 than the registers.
 
 - Saam
 
> On Feb 21, 2017, at 5:43 PM, Mark Lam  wrote:
> 
> Is there a reason why RELEASE_ASSERT (and friends) does not call 
> WTFReportAssertionFailure() to report where the assertion occur?  Is this 
> purely to save memory?  svn blame tells me that it has been this way 
> since the introduction of RELEASE_ASSERT in r140577 many years ago.
> 
> Would anyone object to adding a call to WTFReportAssertionFailure() in 
> RELEASE_ASSERT() like we do for ASSERT()?  One of the upside 
> (side-effect) of adding this call is that it appears to stop the compiler 
> from aggregating all the RELEASE_ASSERTS into a single code location, and 
> this will help with post-mortem crash debugging.
> 
> Any thoughts?
> 
> Mark
> 
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev
 
>>> 
>>> ___
>>> webkit-dev mailing list
>>> webkit-dev@lists.webkit.org
>>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>> 
>> ___
>> webkit-dev mailing list
>> webkit-dev@lists.webkit.org
>> https://lists.webkit.org/mailman/listinfo/webkit-dev
> 

___
webkit-dev mailing list
webkit-dev@lists.webkit.org

Re: [webkit-dev] Why does RELEASE_ASSERT not have an error message?

[Michael Catanzaro]

On Wed, 2017-02-22 at 11:58 -0800, Geoffrey Garen wrote:
> (2) Change RELEASE_ASSERT to do the normal debug ASSERT thing in
> Debug builds. (There’s not much need to preserve register state in
> debug builds.)

This seems like a more desirable approach.

Michael
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Why does RELEASE_ASSERT not have an error message?

[Saam barati]

I like this idea. I think even doing what JF suggested could be really nice for 
suspected crashes. That way the register state would just tell us the reason.
If we do log, I think we should use WTFLogAlways instead of dataLog, because I 
think that does show up in some crash logs (but I could be wrong about this, 
I’m not 100% sure).

- Saam

> On Feb 22, 2017, at 11:58 AM, Geoffrey Garen  wrote:
> 
> I’ve lost countless hours to investigating CrashTracers that would have been 
> easy to solve if I had access to register state.
> 
> I also want the freedom to add RELEASE_ASSERT without ruining performance due 
> to bad register allocation or making the code too large to inline. For 
> example, hot paths in WTF::Vector use RELEASE_ASSERT.
> 
> Is some compromise solution possible?
> 
> Some options:
> 
> (1) Add a variant of RELEASE_ASSERT that takes a string and logs.
> 
> (2) Change RELEASE_ASSERT to do the normal debug ASSERT thing in Debug 
> builds. (There’s not much need to preserve register state in debug builds.)
> 
> Geoff
> 
>> On Feb 22, 2017, at 11:09 AM, Filip Pizlo  wrote:
>> 
>> I disagree actually.  I've lost countless hours to converting this:
>> 
>> RELEASE_ASSERT(blah)
>> 
>> into this:
>> 
>> if (!blah) {
>>  dataLog("Reason why I crashed");
>>  RELEASE_ASSERT_NOT_REACHED();
>> }
>> 
>> Look in the code - you'll find lots of stuff like this.
>> 
>> I don't think analyzing register state at crashes is more important than 
>> keeping our code sane.
>> 
>> -Filip
>> 
>> 
>>> On Feb 21, 2017, at 5:56 PM, Mark Lam  wrote:
>>> 
>>> Oh yeah, I forgot about that.  I think the register state is more important 
>>> for crash analysis, especially if we can make sure that the compiler does 
>>> not aggregate the int3s.  I’ll explore alternatives.
>>> 
 On Feb 21, 2017, at 5:54 PM, Saam barati  wrote:
 
 I thought the main point of moving to SIGTRAP was to preserve register 
 state?
 
 That said, there are probably places where we care more about the message 
 than the registers.
 
 - Saam
 
> On Feb 21, 2017, at 5:43 PM, Mark Lam  wrote:
> 
> Is there a reason why RELEASE_ASSERT (and friends) does not call 
> WTFReportAssertionFailure() to report where the assertion occur?  Is this 
> purely to save memory?  svn blame tells me that it has been this way 
> since the introduction of RELEASE_ASSERT in r140577 many years ago.
> 
> Would anyone object to adding a call to WTFReportAssertionFailure() in 
> RELEASE_ASSERT() like we do for ASSERT()?  One of the upside 
> (side-effect) of adding this call is that it appears to stop the compiler 
> from aggregating all the RELEASE_ASSERTS into a single code location, and 
> this will help with post-mortem crash debugging.
> 
> Any thoughts?
> 
> Mark
> 
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev
 
>>> 
>>> ___
>>> webkit-dev mailing list
>>> webkit-dev@lists.webkit.org
>>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>> 
>> ___
>> webkit-dev mailing list
>> webkit-dev@lists.webkit.org
>> https://lists.webkit.org/mailman/listinfo/webkit-dev
> 
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Why does RELEASE_ASSERT not have an error message?

[Geoffrey Garen]

I’ve lost countless hours to investigating CrashTracers that would have been 
easy to solve if I had access to register state.

I also want the freedom to add RELEASE_ASSERT without ruining performance due 
to bad register allocation or making the code too large to inline. For example, 
hot paths in WTF::Vector use RELEASE_ASSERT.

Is some compromise solution possible?

Some options:

(1) Add a variant of RELEASE_ASSERT that takes a string and logs.

(2) Change RELEASE_ASSERT to do the normal debug ASSERT thing in Debug builds. 
(There’s not much need to preserve register state in debug builds.)

Geoff

> On Feb 22, 2017, at 11:09 AM, Filip Pizlo  wrote:
> 
> I disagree actually.  I've lost countless hours to converting this:
> 
> RELEASE_ASSERT(blah)
> 
> into this:
> 
> if (!blah) {
>   dataLog("Reason why I crashed");
>   RELEASE_ASSERT_NOT_REACHED();
> }
> 
> Look in the code - you'll find lots of stuff like this.
> 
> I don't think analyzing register state at crashes is more important than 
> keeping our code sane.
> 
> -Filip
> 
> 
>> On Feb 21, 2017, at 5:56 PM, Mark Lam  wrote:
>> 
>> Oh yeah, I forgot about that.  I think the register state is more important 
>> for crash analysis, especially if we can make sure that the compiler does 
>> not aggregate the int3s.  I’ll explore alternatives.
>> 
>>> On Feb 21, 2017, at 5:54 PM, Saam barati  wrote:
>>> 
>>> I thought the main point of moving to SIGTRAP was to preserve register 
>>> state?
>>> 
>>> That said, there are probably places where we care more about the message 
>>> than the registers.
>>> 
>>> - Saam
>>> 
 On Feb 21, 2017, at 5:43 PM, Mark Lam  wrote:
 
 Is there a reason why RELEASE_ASSERT (and friends) does not call 
 WTFReportAssertionFailure() to report where the assertion occur?  Is this 
 purely to save memory?  svn blame tells me that it has been this way since 
 the introduction of RELEASE_ASSERT in r140577 many years ago.
 
 Would anyone object to adding a call to WTFReportAssertionFailure() in 
 RELEASE_ASSERT() like we do for ASSERT()?  One of the upside (side-effect) 
 of adding this call is that it appears to stop the compiler from 
 aggregating all the RELEASE_ASSERTS into a single code location, and this 
 will help with post-mortem crash debugging.
 
 Any thoughts?
 
 Mark
 
 ___
 webkit-dev mailing list
 webkit-dev@lists.webkit.org
 https://lists.webkit.org/mailman/listinfo/webkit-dev
>>> 
>> 
>> ___
>> webkit-dev mailing list
>> webkit-dev@lists.webkit.org
>> https://lists.webkit.org/mailman/listinfo/webkit-dev
> 
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Why does RELEASE_ASSERT not have an error message?

[JF Bastien]

Do we get the dataLog output in a crash report? It seems useful to have at
a minimum some "reason" enum which ends up in a register, so the crash
report is somewhat helpful, like we do with JIT code.

At that point the release assert might as well capture __LINE__ and other
things in a register, so that crash reporter picks it up.

Not that the dataLog isn't useful -- it is! -- just not as useful if it
isn't in crash reports.

On Wed, Feb 22, 2017 at 11:09 AM, Filip Pizlo  wrote:

> I disagree actually.  I've lost countless hours to converting this:
>
> RELEASE_ASSERT(blah)
>
> into this:
>
> if (!blah) {
>dataLog("Reason why I crashed");
>RELEASE_ASSERT_NOT_REACHED();
> }
>
> Look in the code - you'll find lots of stuff like this.
>
> I don't think analyzing register state at crashes is more important than
> keeping our code sane.
>
> -Filip
>
>
> > On Feb 21, 2017, at 5:56 PM, Mark Lam  wrote:
> >
> > Oh yeah, I forgot about that.  I think the register state is more
> important for crash analysis, especially if we can make sure that the
> compiler does not aggregate the int3s.  I’ll explore alternatives.
> >
> >> On Feb 21, 2017, at 5:54 PM, Saam barati  wrote:
> >>
> >> I thought the main point of moving to SIGTRAP was to preserve register
> state?
> >>
> >> That said, there are probably places where we care more about the
> message than the registers.
> >>
> >> - Saam
> >>
> >>> On Feb 21, 2017, at 5:43 PM, Mark Lam  wrote:
> >>>
> >>> Is there a reason why RELEASE_ASSERT (and friends) does not call
> WTFReportAssertionFailure() to report where the assertion occur?  Is this
> purely to save memory?  svn blame tells me that it has been this way since
> the introduction of RELEASE_ASSERT in r140577 many years ago.
> >>>
> >>> Would anyone object to adding a call to WTFReportAssertionFailure() in
> RELEASE_ASSERT() like we do for ASSERT()?  One of the upside (side-effect)
> of adding this call is that it appears to stop the compiler from
> aggregating all the RELEASE_ASSERTS into a single code location, and this
> will help with post-mortem crash debugging.
> >>>
> >>> Any thoughts?
> >>>
> >>> Mark
> >>>
> >>> ___
> >>> webkit-dev mailing list
> >>> webkit-dev@lists.webkit.org
> >>> https://lists.webkit.org/mailman/listinfo/webkit-dev
> >>
> >
> > ___
> > webkit-dev mailing list
> > webkit-dev@lists.webkit.org
> > https://lists.webkit.org/mailman/listinfo/webkit-dev
>
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev
>
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Why does RELEASE_ASSERT not have an error message?

[Filip Pizlo]

I disagree actually.  I've lost countless hours to converting this:

RELEASE_ASSERT(blah)

into this:

if (!blah) {
   dataLog("Reason why I crashed");
   RELEASE_ASSERT_NOT_REACHED();
}

Look in the code - you'll find lots of stuff like this.

I don't think analyzing register state at crashes is more important than 
keeping our code sane.

-Filip


> On Feb 21, 2017, at 5:56 PM, Mark Lam  wrote:
> 
> Oh yeah, I forgot about that.  I think the register state is more important 
> for crash analysis, especially if we can make sure that the compiler does not 
> aggregate the int3s.  I’ll explore alternatives.
> 
>> On Feb 21, 2017, at 5:54 PM, Saam barati  wrote:
>> 
>> I thought the main point of moving to SIGTRAP was to preserve register state?
>> 
>> That said, there are probably places where we care more about the message 
>> than the registers.
>> 
>> - Saam
>> 
>>> On Feb 21, 2017, at 5:43 PM, Mark Lam  wrote:
>>> 
>>> Is there a reason why RELEASE_ASSERT (and friends) does not call 
>>> WTFReportAssertionFailure() to report where the assertion occur?  Is this 
>>> purely to save memory?  svn blame tells me that it has been this way since 
>>> the introduction of RELEASE_ASSERT in r140577 many years ago.
>>> 
>>> Would anyone object to adding a call to WTFReportAssertionFailure() in 
>>> RELEASE_ASSERT() like we do for ASSERT()?  One of the upside (side-effect) 
>>> of adding this call is that it appears to stop the compiler from 
>>> aggregating all the RELEASE_ASSERTS into a single code location, and this 
>>> will help with post-mortem crash debugging.
>>> 
>>> Any thoughts?
>>> 
>>> Mark
>>> 
>>> ___
>>> webkit-dev mailing list
>>> webkit-dev@lists.webkit.org
>>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>> 
> 
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Implementing Universal Second Factor (U2F)

[Michael Catanzaro]

On Wed, 2017-02-22 at 12:20 -0500, Jacob Greenfield wrote:
> Michael,
> 
> Thanks for the info. After looking into things a bit more - let me
> know if this does not make sense - it looks like it may be better to
> reimplement for WebKit.

OK, your explanation sounds reasonable to me.

Michael
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Implementing Universal Second Factor (U2F)

[Jacob Greenfield]

Michael,

Thanks for the info. After looking into things a bit more - let me know if this 
does not make sense - it looks like it may be better to reimplement for WebKit.

Specifically, it looks like hidapi does some opaque stuff with threading that 
may leave idle threads around, to expose a consistent API across platforms. 

The other reason (and an area I haven't explored much yet) is that much of the 
code in libu2f-host doesn't seem to be concerned with the protocol itself, but 
with other details of the protocol - stuff that WebKit already has; the biggest 
two I see are (and it looks like another dependency on JSON-C here) formatting 
browser responses and base64 encoding.

On a cursory glance at their code for reading a response from the device, it 
looks like 2 threads will have to be spawned: one to allow libu2f-host to block 
(it calls sleep if the device isn't ready) on a response from hidapi, and one 
for hidapi to monitor the device.

It looks like doing a clean implementation for Linux should also not be too 
hard; I think it would be okay to only enable it if hidraw is available, which 
should be the case now on most shipping kernels for desktop distros.

The Windows API also seems to have some HID stuff; I can look into this if any 
ports which would enable this feature want Windows support.

- Jacob Greenfield

> On Feb 22, 2017, at 10:17, Michael Catanzaro  wrote:
> 
>> On Wed, 2017-02-22 at 08:52 -0500, Jacob Greenfield wrote:
>> The (USB) protocol itself works by sending USB HID reports (packets)
>> to the key and getting USB HID reports back. There is a reference
>> implementation by one of the members of the specification group -
>> libu2f-host (by Yubico); however, it is licensed under GPL and
>> LGPLv2.1.
> 
> Hi,
> 
> According to the documentation:
> 
> "The library and command-ine tool is licensed under the LGPLv2+
> license. Some other files are licensed under the GPLv3+ license."
> 
> Sounds like a perfectly acceptable dependency for WebKit. The only
> trick is that if you need to import the source code under
> Source/ThirdParty (hopefully not), then you'd need to remove all the
> GPLv3+ files from the repository.
> 
>> - What to do about other platforms - no implementation, use libu2f-
>> host for them, or use libu2f-host everywhere
> 
> Ideally we would use the same code on all platforms unless there is a
> strong reason not to do so, to reduce the amount of platform-specific
> bugs. I'm not sure if there exists such a situation here, so using
> libu2f-host on all platforms seems to make the most sense. Possible
> examples of strong reasons to diverge:
> 
> * If removing the GPLv3+ files would be somehow problematic. GTK+ port
> doesn't care so long as none of the GPL files are used by the library
> itself. Apple is much stricter; I guess they might not allow the files
> to be imported into internal build systems. That could create a
> maintenance headache.
> * If the extra dependencies are problematic for some platform. You'd
> be hard-pressed to find a Linux system without libusb, and hidapi and
> libu2f-host both look unproblematic. But adding your implementation
> directly to WebKit might be easier for Mac ports.
> 
>> - UI for key access permission - malicious sites could (eventually)
>> lock up a key, as well as possibly identifying a user; the
>> specification suggests displaying an info bar for user to allow
>> access - but, I’m not familiar with the process of designing/adding
>> browser chromes
> 
> Sounds like WebKit should request a policy decision from the client on
> whether to allow a website to use the U2F API? Then making that
> decision in a secure manner is the responsibility of the client to
> figure out. In Epiphany we display info bars the first time a
> particular security origin requests a permission, and remember the
> user's choice in the future.
> 
>> - What process should communicate with the token - the protocol is
>> robust and designed for many simultaneous accesses and appropriate
>> isolation of them, so this can (should?) be per-page; IOKit needs a
>> CFRunLoop to schedule the report receive callback on: should this be
>> on the main runloop or on another thread just for U2F?
> 
> I don't know which process, but you should use the main runloop. Let's
> avoid creating a new always-running thread for a feature that will
> rarely be used.
> 
>> - Presumably, this should be gated behind a macro; does a suitable
>> one exist, or add a new one?
> 
> You'll have to add a new one, ENABLE_U2F. It's better not to gate
> features when possible, but in this case one will be required due to
> the new dependencies as we want to allow distributions that do not have
> the new dependencies to continue to build without them (with reduced
> feature set) for the next few years.
> 
> Michael

___
webkit-dev mailing list
webkit-dev@lists.webkit.org

Re: [webkit-dev] Webkit port to ppc64le.

[Filip Pizlo]

You have to port the llint either way since even the JIT relies on some llint 
code. 

-Filip

> On Feb 22, 2017, at 07:22, Atul Sowani  wrote:
> 
> Sure, thanks @Konstantin. So I will first attempt having working JIT on 
> ppc64le. Your comments definitely helped in deciding the direction for 
> porting. Thanks! :)
> 
>> On Wed, Feb 22, 2017 at 8:28 PM, Konstantin Tokarev  
>> wrote:
>> 
>> 
>> 22.02.2017, 17:41, "Atul Sowani" :
>> > So, essentially, some tweaking in the code of LLInt/interpreter _is_ 
>> > required when porting it to the new platform. Is my understanding correct?
>> 
>> Yes. In order to port LLInt you should implement offlineasm backend. See for 
>> example arm.rb, arm64.rb, mips.rb in Source/JavaScriptCore/offlineasm/. You 
>> may also need arch-specific adjustements in Source/JavaScriptCore/llint, 
>> though it's better to minimize their number.
>> 
>> My point was that you don't have to port LLInt before you have working JIT. 
>> AFAIU main purpose of architecture-specific LLInt implementation is to 
>> provide interpreter with the same calling convention as JIT, not to make 
>> interpreter faster on its own.
>> 
>> See also https://trac.webkit.org/wiki/JavaScriptCore
>> 
>> > On Wed, Feb 22, 2017 at 4:11 PM, Konstantin Tokarev  
>> > wrote:
>> >
>> >> 22.02.2017, 13:15, "Atul Sowani" :
>> >>> Hi,
>> >>>
>> >>> This is not specific to any particular branch/version of WebKit. I was 
>> >>> browsing the code with ppc64le porting in mind. By default JIT is not 
>> >>> available on ppc64le, so the CLoop code is used instead.
>> >>>
>> >>> I see there is low level interpreter code in 
>> >>> qtwebkit/Source/JavaScriptCore/llint directory and inside 
>> >>> qtwebkit/Source/JavaScriptCore/interpreter directory (AbstractPC, 
>> >>> CallFrame etc.). I am wondering if one needs to touch this code as well 
>> >>> to make it work correctly on ppc64le.
>> >>
>> >> Porting JIT to new platfrom does not require porting LLInt, it can work 
>> >> without interpreter tier. However, it is a good idea to port LLInt after 
>> >> you have JIT in place, to improve overall quality.
>> >>
>> >>>
>> >>> Any comments/suggestions?
>> >>>
>> >>> Thanks,
>> >>> Atul.
>> >>> ,
>> >>>
>> >>> ___
>> >>> webkit-dev mailing list
>> >>> webkit-dev@lists.webkit.org
>> >>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>> >>
>> >> --
>> >> Regards,
>> >> Konstantin
>> > ,
>> >
>> > ___
>> > webkit-dev mailing list
>> > webkit-dev@lists.webkit.org
>> > https://lists.webkit.org/mailman/listinfo/webkit-dev
>> 
>> 
>> -- 
>> Regards,
>> Konstantin
> 
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Webkit port to ppc64le.

[Atul Sowani]

Sure, thanks @Konstantin. So I will first attempt having working JIT on
ppc64le. Your comments definitely helped in deciding the direction for
porting. Thanks! :)

On Wed, Feb 22, 2017 at 8:28 PM, Konstantin Tokarev 
wrote:

>
>
> 22.02.2017, 17:41, "Atul Sowani" :
> > So, essentially, some tweaking in the code of LLInt/interpreter _is_
> required when porting it to the new platform. Is my understanding correct?
>
> Yes. In order to port LLInt you should implement offlineasm backend. See
> for example arm.rb, arm64.rb, mips.rb in Source/JavaScriptCore/offlineasm/.
> You may also need arch-specific adjustements in
> Source/JavaScriptCore/llint, though it's better to minimize their number.
>
> My point was that you don't have to port LLInt before you have working
> JIT. AFAIU main purpose of architecture-specific LLInt implementation is to
> provide interpreter with the same calling convention as JIT, not to make
> interpreter faster on its own.
>
> See also https://trac.webkit.org/wiki/JavaScriptCore
>
> > On Wed, Feb 22, 2017 at 4:11 PM, Konstantin Tokarev 
> wrote:
> >
> >> 22.02.2017, 13:15, "Atul Sowani" :
> >>> Hi,
> >>>
> >>> This is not specific to any particular branch/version of WebKit. I was
> browsing the code with ppc64le porting in mind. By default JIT is not
> available on ppc64le, so the CLoop code is used instead.
> >>>
> >>> I see there is low level interpreter code in 
> >>> qtwebkit/Source/JavaScriptCore/llint
> directory and inside qtwebkit/Source/JavaScriptCore/interpreter directory
> (AbstractPC, CallFrame etc.). I am wondering if one needs to touch this
> code as well to make it work correctly on ppc64le.
> >>
> >> Porting JIT to new platfrom does not require porting LLInt, it can work
> without interpreter tier. However, it is a good idea to port LLInt after
> you have JIT in place, to improve overall quality.
> >>
> >>>
> >>> Any comments/suggestions?
> >>>
> >>> Thanks,
> >>> Atul.
> >>> ,
> >>>
> >>> ___
> >>> webkit-dev mailing list
> >>> webkit-dev@lists.webkit.org
> >>> https://lists.webkit.org/mailman/listinfo/webkit-dev
> >>
> >> --
> >> Regards,
> >> Konstantin
> > ,
> >
> > ___
> > webkit-dev mailing list
> > webkit-dev@lists.webkit.org
> > https://lists.webkit.org/mailman/listinfo/webkit-dev
>
>
> --
> Regards,
> Konstantin
>
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Implementing Universal Second Factor (U2F)

[Michael Catanzaro]

On Wed, 2017-02-22 at 08:52 -0500, Jacob Greenfield wrote:
> The (USB) protocol itself works by sending USB HID reports (packets)
> to the key and getting USB HID reports back. There is a reference
> implementation by one of the members of the specification group -
> libu2f-host (by Yubico); however, it is licensed under GPL and
> LGPLv2.1.

Hi,

According to the documentation:

"The library and command-ine tool is licensed under the LGPLv2+
license. Some other files are licensed under the GPLv3+ license."

Sounds like a perfectly acceptable dependency for WebKit. The only
trick is that if you need to import the source code under
Source/ThirdParty (hopefully not), then you'd need to remove all the
GPLv3+ files from the repository.

> - What to do about other platforms - no implementation, use libu2f-
> host for them, or use libu2f-host everywhere

Ideally we would use the same code on all platforms unless there is a
strong reason not to do so, to reduce the amount of platform-specific
bugs. I'm not sure if there exists such a situation here, so using
libu2f-host on all platforms seems to make the most sense. Possible
examples of strong reasons to diverge:

 * If removing the GPLv3+ files would be somehow problematic. GTK+ port
doesn't care so long as none of the GPL files are used by the library
itself. Apple is much stricter; I guess they might not allow the files
to be imported into internal build systems. That could create a
maintenance headache.
 * If the extra dependencies are problematic for some platform. You'd
be hard-pressed to find a Linux system without libusb, and hidapi and
libu2f-host both look unproblematic. But adding your implementation
directly to WebKit might be easier for Mac ports.

> - UI for key access permission - malicious sites could (eventually)
> lock up a key, as well as possibly identifying a user; the
> specification suggests displaying an info bar for user to allow
> access - but, I’m not familiar with the process of designing/adding
> browser chromes

Sounds like WebKit should request a policy decision from the client on
whether to allow a website to use the U2F API? Then making that
decision in a secure manner is the responsibility of the client to
figure out. In Epiphany we display info bars the first time a
particular security origin requests a permission, and remember the
user's choice in the future.

> - What process should communicate with the token - the protocol is
> robust and designed for many simultaneous accesses and appropriate
> isolation of them, so this can (should?) be per-page; IOKit needs a
> CFRunLoop to schedule the report receive callback on: should this be
> on the main runloop or on another thread just for U2F?

I don't know which process, but you should use the main runloop. Let's
avoid creating a new always-running thread for a feature that will
rarely be used.

> - Presumably, this should be gated behind a macro; does a suitable
> one exist, or add a new one?

You'll have to add a new one, ENABLE_U2F. It's better not to gate
features when possible, but in this case one will be required due to
the new dependencies as we want to allow distributions that do not have
the new dependencies to continue to build without them (with reduced
feature set) for the next few years.

Michael
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] ReadMe.md on Github

[Michael Catanzaro]

On Tue, 2017-02-21 at 22:53 -0800, Ryosuke Niwa wrote:
> I feel like GTK+ port's instruction can be consolidated enough to fit
> there but we might want to write some script that someone can run to
> do everything you need like Tools/Scripts/configure-xcode-for-ios-
> development for iOS first.

Currently there are just two scripts (install-dependencies and update-
webkitgtk-libs), and they are sufficiently-different that they should
not be consolidated.

That said, a pointer from the README to somewhere else is fine for us. 
Certainly that's the way to go if the goal is to reduce the size of the
README. I would go as far to remove the Mac/iOS build and run
instructions and just point to the website for all ports. The more
content we duplicate in the README, the more content we have that will
get out of sync with the website.

Michael
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Webkit port to ppc64le.

[Konstantin Tokarev]

22.02.2017, 17:41, "Atul Sowani" :
> So, essentially, some tweaking in the code of LLInt/interpreter _is_ required 
> when porting it to the new platform. Is my understanding correct?

Yes. In order to port LLInt you should implement offlineasm backend. See for 
example arm.rb, arm64.rb, mips.rb in Source/JavaScriptCore/offlineasm/. You may 
also need arch-specific adjustements in Source/JavaScriptCore/llint, though 
it's better to minimize their number.

My point was that you don't have to port LLInt before you have working JIT. 
AFAIU main purpose of architecture-specific LLInt implementation is to provide 
interpreter with the same calling convention as JIT, not to make interpreter 
faster on its own.

See also https://trac.webkit.org/wiki/JavaScriptCore

> On Wed, Feb 22, 2017 at 4:11 PM, Konstantin Tokarev  wrote:
>
>> 22.02.2017, 13:15, "Atul Sowani" :
>>> Hi,
>>>
>>> This is not specific to any particular branch/version of WebKit. I was 
>>> browsing the code with ppc64le porting in mind. By default JIT is not 
>>> available on ppc64le, so the CLoop code is used instead.
>>>
>>> I see there is low level interpreter code in 
>>> qtwebkit/Source/JavaScriptCore/llint directory and inside 
>>> qtwebkit/Source/JavaScriptCore/interpreter directory (AbstractPC, CallFrame 
>>> etc.). I am wondering if one needs to touch this code as well to make it 
>>> work correctly on ppc64le.
>>
>> Porting JIT to new platfrom does not require porting LLInt, it can work 
>> without interpreter tier. However, it is a good idea to port LLInt after you 
>> have JIT in place, to improve overall quality.
>>
>>>
>>> Any comments/suggestions?
>>>
>>> Thanks,
>>> Atul.
>>> ,
>>>
>>> ___
>>> webkit-dev mailing list
>>> webkit-dev@lists.webkit.org
>>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>>
>> --
>> Regards,
>> Konstantin
> ,
>
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev


-- 
Regards,
Konstantin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Webkit port to ppc64le.

[Atul Sowani]

So, essentially, some tweaking in the code of LLInt/interpreter _is_
required when porting it to the new platform. Is my understanding correct?

On Wed, Feb 22, 2017 at 4:11 PM, Konstantin Tokarev 
wrote:

>
>
> 22.02.2017, 13:15, "Atul Sowani" :
> > Hi,
> >
> > This is not specific to any particular branch/version of WebKit. I was
> browsing the code with ppc64le porting in mind. By default JIT is not
> available on ppc64le, so the CLoop code is used instead.
> >
> > I see there is low level interpreter code in 
> > qtwebkit/Source/JavaScriptCore/llint
> directory and inside qtwebkit/Source/JavaScriptCore/interpreter directory
> (AbstractPC, CallFrame etc.). I am wondering if one needs to touch this
> code as well to make it work correctly on ppc64le.
>
> Porting JIT to new platfrom does not require porting LLInt, it can work
> without interpreter tier. However, it is a good idea to port LLInt after
> you have JIT in place, to improve overall quality.
>
> >
> > Any comments/suggestions?
> >
> > Thanks,
> > Atul.
> > ,
> >
> > ___
> > webkit-dev mailing list
> > webkit-dev@lists.webkit.org
> > https://lists.webkit.org/mailman/listinfo/webkit-dev
>
>
> --
> Regards,
> Konstantin
>
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

[webkit-dev] Implementing Universal Second Factor (U2F)

[Jacob Greenfield]

I’m working on adding support to WebKit for FIDO U2F (JS API: 
https://fidoalliance.org/specs/fido-u2f-v1.1-id-20160915/fido-u2f-javascript-api-v1.1-id-20160915.html
 Architecture overview: 
https://fidoalliance.org/specs/fido-u2f-v1.1-id-20160915/fido-u2f-overview-v1.1-id-20160915.html
 ). The FIDO U2F specification allows a secure second factor to be used during 
authentication flow, with bidirectional verification (token verifies server, 
server verifies token and token’s knowledge of a specific private key). There 
are current implementations in Chrome, Opera, and Blink (Firefox). I’m 
primarily interested in bringing support to Safari, so that is the focus what I 
am currently working on.

The (USB) protocol itself works by sending USB HID reports (packets) to the key 
and getting USB HID reports back. There is a reference implementation by one of 
the members of the specification group - libu2f-host (by Yubico); however, it 
is licensed under GPL and LGPLv2.1. It also depends on two more libraries, 
hidapi and libusb. Figuring that adding all of these dependencies to Safari 
might be undesirable, I wrote a clean-room implementation outside of WebKit 
that uses IOKit directly to access the device (conveniently, IOKit exposes nice 
HID stuff). I’m now at the stage of adding this to WebKit.

Before I move forward, there are a couple of things that would be great to get 
some input on:

- What to do about other platforms - no implementation, use libu2f-host for 
them, or use libu2f-host everywhere
- UI for key access permission - malicious sites could (eventually) lock up a 
key, as well as possibly identifying a user; the specification suggests 
displaying an info bar for user to allow access - but, I’m not familiar with 
the process of designing/adding browser chromes
- What process should communicate with the token - the protocol is robust and 
designed for many simultaneous accesses and appropriate isolation of them, so 
this can (should?) be per-page; IOKit needs a CFRunLoop to schedule the report 
receive callback on: should this be on the main runloop or on another thread 
just for U2F?
- Presumably, this should be gated behind a macro; does a suitable one exist, 
or add a new one?

Thank you!

- Jacob Greenfield
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Webkit port to ppc64le.

[Konstantin Tokarev]

22.02.2017, 13:15, "Atul Sowani" :
> Hi,
>
> This is not specific to any particular branch/version of WebKit. I was 
> browsing the code with ppc64le porting in mind. By default JIT is not 
> available on ppc64le, so the CLoop code is used instead.
>
> I see there is low level interpreter code in 
> qtwebkit/Source/JavaScriptCore/llint directory and inside 
> qtwebkit/Source/JavaScriptCore/interpreter directory (AbstractPC, CallFrame 
> etc.). I am wondering if one needs to touch this code as well to make it work 
> correctly on ppc64le.

Porting JIT to new platfrom does not require porting LLInt, it can work without 
interpreter tier. However, it is a good idea to port LLInt after you have JIT 
in place, to improve overall quality.

>
> Any comments/suggestions?
>
> Thanks,
> Atul.
> ,
>
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev


-- 
Regards,
Konstantin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

[webkit-dev] Webkit port to ppc64le.

[Atul Sowani]

Hi,

This is not specific to any particular branch/version of WebKit. I was
browsing the code with ppc64le porting in mind. By default JIT is not
available on ppc64le, so the CLoop code is used instead.

I see there is low level interpreter code in
qtwebkit/Source/JavaScriptCore/llint directory and inside
qtwebkit/Source/JavaScriptCore/interpreter directory (AbstractPC, CallFrame
etc.). I am wondering if one needs to touch this code as well to make it
work correctly on ppc64le.

Any comments/suggestions?

Thanks,
Atul.
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev