5 ms·

Edit: Someone has explained the issue below I don't understand how that broke anything? The C++ codegen for && and & is the same: bool good(bool a, bool

Edit: Someone has explained the issue below I don't understand how that broke anything? The C++ codegen for && and & is the same: bool good(bool a, bool b) { return a && b; } good(bool, bool): mov eax, edi and eax, esi ret bool bad(bool a, bool b) { return a & b; } bad(bool, bool): mov eax, edi and eax, esi ret [0] https://godbolt.org/z/84MMqTehs https://godbolt.org/z/84MMqTehs

taeric 5y ago
The short circuiting was preventing undefined behavior on the second expression. That is, yes, it is the same with booleans. Now try with expressions.
formerly_proven 5y ago
The code is: key_data.has_value() && !key_data_->label().empty() key_data is probably something like an std::optional, so short-circuit evaluation guards the dereferencing of the optional. If you write this with a binary and, no short-circuit evaluation can happen, so undefined behavior ensues. I wanna point out that std::optional, in the typical C++ stance, has a "I know what I'm doing" API and a "Humans make mistakes API": optional->... is UB if the optional is empty, optional.value()... raises an exception. Arguably, in code like this you probably only want to use the latter kind of API. (The pattern of "operator can UB, equivalent method asserts pre-condition" is very common in the C++ standard library)
- jeffbee 5y ago
  Correct, key_data_ is a base::Optional<KeyData>. Chromium's base::Optional is very similar to std::optional, but predates c++17.
- bialpio 5y ago
  Except that Google uses C++ with exceptions disabled - IIUC, the throwing code would effectively behave like a call to std::terminate(), I haven't looked at the code to know if it would actually be a preferred behavior in this case. Source: I'm a Chrome contributor.
- inshadows 5y ago
  Where is the undefined behavior? AFAU, UB is a compile time concept. As other commenter has asked, how can the compiler infer that has_value() call has any relation to key_data_ non-NULL-ness? EDIT: > optional->... is UB if the optional is empty I understand what you mean: dereferencing optional is undefined if has_value() is not checked for true in the same thread before. But that undefined behavior happens at runtime, i.e. compile time UB is not invoked, the statements are kept without any optimizations. Right? The terminology is confusing.
  - pornel 5y ago
    UB is not an event that happens. UB is a rule that allows compilers to assume that situations defined as UB never happen. It's UB to access an optional that has no value, so if the compiler sees optional accessed unconditionally, it may assume the optional must have had a value, and delete all code that contradicts this conclusion. Compilers don't do this out of spite. It's an optimization that removes redundant checks (e.g. when code in an inline function checks args != null, but you use the function with obviously non-null args).
    - inshadows 5y ago
      Yeah, I know what UB is, and that's why I'm saying that this is not UB. Have you even read what I wrote?
      - tialaramex 5y ago
        I recommend stepping back for a moment and asking yourself: Why is it that you, the person who believe they "know what UB is" and is sure "this is not UB" did not understand what's going on here, while the other people reading who explained why it's UB have understood exactly what's going on ? Doesn't it seem most likely that you in fact do not understand Undefined Behaviour and didn't understand what's going on ?
        inshadows 5y ago
        I don't think you understand UB, because clearly, this is not UB. But I congratulate you. You have enough knowledge to engage in Internet discussions involving fancy names without understanding underlying concepts. Hint: 1) Read my original post. 2) Contemplate whether the compiler is removing anything in this instance.
tmiahm 5y ago
The difference is your example uses bool values, the code in question uses expressions that (should) evaluate to bool. && is short-circuited if a is false, such that b is not evaluated.
deleted 5y ago
[deleted]
magpie09 5y ago
I made what I think is a minimal example here: https://godbolt.org/z/dcnzonjar https://godbolt.org/z/dcnzonjar Interestingly I couldn't get it to fail on GCC (even with -fnoexceptions and -Ofast), only on Clang. Maybe someone else has some info as to why this is? There is a clear difference in the assembly output for the correct and incorrect implementations, with a testb instruction run much earlier in the correct version. You can see this at line 22 of the output, if you highlight it the && in the source is highlighted in bold.

taeric 5y ago

The short circuiting was preventing undefined behavior on the second expression. That is, yes, it is the same with booleans. Now try with expressions.

formerly_proven 5y ago

The code is: key_data.has_value() && !key_data_->label().empty() key_data is probably something like an std::optional, so short-circuit evaluation guards the dereferencing of the optional. If you write this with a binary and, no short-circuit evaluation can happen, so undefined behavior ensues. I wanna point out that std::optional, in the typical C++ stance, has a "I know what I'm doing" API and a "Humans make mistakes API": optional->... is UB if the optional is empty, optional.value()... raises an exception. Arguably, in code like this you probably only want to use the latter kind of API. (The pattern of "operator can UB, equivalent method asserts pre-condition" is very common in the C++ standard library)

jeffbee 5y ago
Correct, key_data_ is a base::Optional<KeyData>. Chromium's base::Optional is very similar to std::optional, but predates c++17.
bialpio 5y ago
Except that Google uses C++ with exceptions disabled - IIUC, the throwing code would effectively behave like a call to std::terminate(), I haven't looked at the code to know if it would actually be a preferred behavior in this case. Source: I'm a Chrome contributor.
inshadows 5y ago
Where is the undefined behavior? AFAU, UB is a compile time concept. As other commenter has asked, how can the compiler infer that has_value() call has any relation to key_data_ non-NULL-ness? EDIT: > optional->... is UB if the optional is empty I understand what you mean: dereferencing optional is undefined if has_value() is not checked for true in the same thread before. But that undefined behavior happens at runtime, i.e. compile time UB is not invoked, the statements are kept without any optimizations. Right? The terminology is confusing.
- pornel 5y ago
  UB is not an event that happens. UB is a rule that allows compilers to assume that situations defined as UB never happen. It's UB to access an optional that has no value, so if the compiler sees optional accessed unconditionally, it may assume the optional must have had a value, and delete all code that contradicts this conclusion. Compilers don't do this out of spite. It's an optimization that removes redundant checks (e.g. when code in an inline function checks args != null, but you use the function with obviously non-null args).
  - inshadows 5y ago
    Yeah, I know what UB is, and that's why I'm saying that this is not UB. Have you even read what I wrote?
    - tialaramex 5y ago
      I recommend stepping back for a moment and asking yourself: Why is it that you, the person who believe they "know what UB is" and is sure "this is not UB" did not understand what's going on here, while the other people reading who explained why it's UB have understood exactly what's going on ? Doesn't it seem most likely that you in fact do not understand Undefined Behaviour and didn't understand what's going on ?
      - inshadows 5y ago
        I don't think you understand UB, because clearly, this is not UB. But I congratulate you. You have enough knowledge to engage in Internet discussions involving fancy names without understanding underlying concepts. Hint: 1) Read my original post. 2) Contemplate whether the compiler is removing anything in this instance.

tmiahm 5y ago

The difference is your example uses bool values, the code in question uses expressions that (should) evaluate to bool. && is short-circuited if a is false, such that b is not evaluated.

deleted 5y ago

[deleted]

magpie09 5y ago

I made what I think is a minimal example here: https://godbolt.org/z/dcnzonjar https://godbolt.org/z/dcnzonjar Interestingly I couldn't get it to fail on GCC (even with -fnoexceptions and -Ofast), only on Clang. Maybe someone else has some info as to why this is? There is a clear difference in the assembly output for the correct and incorrect implementations, with a testb instruction run much earlier in the correct version. You can see this at line 22 of the output, if you highlight it the && in the source is highlighted in bold.