[XS] Change in fuchsia/glog[main]: [glog] Add [[clang::lto_visibility_public]]

1 view
Skip to first unread message

'Leonard Chan (Gerrit)' via owners-override

unread,
Apr 24, 2024, 2:28:39 PMApr 24
to Owners Override
Attention needed from Cameron Dale, Owners Override and Steve Fung

Leonard Chan has uploaded the change for review

Leonard Chan would like Owners Override to review this change.

Commit message

[glog] Add [[clang::lto_visibility_public]]

With whole program devirtualization enabled, we see a call to
streambuf::underflow incorrectly devirtualized to
basic_streambuf::underflow() when it should instead not be devirtualized
and a vtable call will result in the correct call to
basic_filebuf::underflow. WPD is doing this because we are statically
linking in the definitions and vtables for these classes from libc++.a.
This is an incorrect use of WPD because the definitions of these classes
are outside the LTO unit being linked, so when we attempt to devirt the
calls, the only visible class in the LTO unit is LogStreamBuf thus WPD
assumes the only possible call that can be made for the call to
underflow is the definition from LogStreamBuf, which directly inherits
from std:streambug (aka basic_streambuf).

The correct way to use WPD would be to compile all relevant TUs with
-flto, but we aren't currently able to do that with the llvm runtimes.
Ideally, FatLTO will help with this since we'd ship both bitcode and
objcode, but that's some ways away. For now, what we can do, without
explicitly making the class have default ELF visibility is to make
LogStreamBuf have public LTO visibility. This means the this class is
accessible outside the LTO unit and can have child classes and cannot be
a candidate for devirting.
Bug: 42075686
Change-Id: I520345493e7047e751c2030081ab42bad95566d3

Change diff

diff --git a/src/glog/logging.h b/src/glog/logging.h
index 300d253..12b43f4 100644
--- a/src/glog/logging.h
+++ b/src/glog/logging.h
@@ -1101,7 +1101,7 @@
// LogMessage::LogStream is a std::ostream backed by this streambuf.
// This class ignores overflow and leaves two bytes at the end of the
// buffer to allow for a '\n' and '\0'.
-class GOOGLE_GLOG_DLL_DECL LogStreamBuf : public std::streambuf {
+class GOOGLE_GLOG_DLL_DECL [[clang::lto_visibility_public]] LogStreamBuf : public std::streambuf {
public:
// REQUIREMENTS: "len" must be >= 2 to account for the '\n' and '\n'.
LogStreamBuf(char *buf, int len) {

Change information

Files:
  • M src/glog/logging.h
Change size: XS
Delta: 1 file changed, 1 insertion(+), 1 deletion(-)
Open in Gerrit

Related details

Attention is currently required from:
  • Cameron Dale
  • Owners Override
  • Steve Fung
Submit Requirements:
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: newchange
Gerrit-Project: third_party/glog
Gerrit-Branch: main
Gerrit-Change-Id: I520345493e7047e751c2030081ab42bad95566d3
Gerrit-Change-Number: 1035172
Gerrit-PatchSet: 1
Gerrit-Owner: Leonard Chan <leona...@google.com>
Gerrit-Reviewer: Cameron Dale <camr...@google.com>
Gerrit-Reviewer: Owners Override <owners-...@fuchsia.dev>
Gerrit-Reviewer: Steve Fung <stev...@google.com>
Gerrit-Attention: Owners Override <owners-...@fuchsia.dev>
Gerrit-Attention: Steve Fung <stev...@google.com>
Gerrit-Attention: Cameron Dale <camr...@google.com>

--
You received this message because you are subscribed to the Google Groups "owners-override" group.
To unsubscribe from this group and stop receiving emails from it, send an email to owners-overri...@fuchsia.dev.
To view this discussion on the web visit https://groups.google.com/a/fuchsia.dev/d/msgid/owners-override/bc7e33a28b986c5c34e85a2fff83609fbd517076-HTML%40fuchsia-review.googlesource.com.
unsatisfied_requirement
open
diffy

'Adam Barth (Gerrit)' via owners-override

unread,
Apr 24, 2024, 3:01:02 PMApr 24
to Leonard Chan, Owners Override, Steve Fung, Cameron Dale
Attention needed from Cameron Dale, Leonard Chan, Owners Override and Steve Fung

Adam Barth added 1 comment

Patchset-level comments
File-level comment, Patchset 1 (Latest):
Adam Barth . resolved

I see that you've added owners-override, but I don't understand why the normal OWNERS aren't sufficient to review this CL.

Open in Gerrit

Related details

Attention is currently required from:
  • Cameron Dale
  • Leonard Chan
  • Owners Override
  • Steve Fung
Submit Requirements:
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: third_party/glog
Gerrit-Branch: main
Gerrit-Change-Id: I520345493e7047e751c2030081ab42bad95566d3
Gerrit-Change-Number: 1035172
Gerrit-PatchSet: 1
Gerrit-Owner: Leonard Chan <leona...@google.com>
Gerrit-Reviewer: Cameron Dale <camr...@google.com>
Gerrit-Reviewer: Owners Override <owners-...@fuchsia.dev>
Gerrit-Reviewer: Steve Fung <stev...@google.com>
Gerrit-CC: Adam Barth <aba...@google.com>
Gerrit-Attention: Leonard Chan <leona...@google.com>
Gerrit-Attention: Owners Override <owners-...@fuchsia.dev>
Gerrit-Attention: Steve Fung <stev...@google.com>
Gerrit-Attention: Cameron Dale <camr...@google.com>
Gerrit-Comment-Date: Wed, 24 Apr 2024 19:00:57 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No

--
You received this message because you are subscribed to the Google Groups "owners-override" group.
To unsubscribe from this group and stop receiving emails from it, send an email to owners-overri...@fuchsia.dev.
To view this discussion on the web visit https://groups.google.com/a/fuchsia.dev/d/msgid/owners-override/fc5040f96d2b5078eb87fd1248c70997898820dc-EmailReviewComments-HTML%40fuchsia-review.googlesource.com.
unsatisfied_requirement
open
diffy

'James Robinson (Gerrit)' via owners-override

unread,
Apr 24, 2024, 3:02:30 PMApr 24
to Leonard Chan, Adam Barth, Owners Override, Steve Fung, Cameron Dale
Attention needed from Cameron Dale, Leonard Chan, Owners Override and Steve Fung

James Robinson added 1 comment

Patchset-level comments
James Robinson . resolved

I don't think we have owners checking enabled in this repository at all.

Open in Gerrit

Related details

Attention is currently required from:
  • Cameron Dale
  • Leonard Chan
  • Owners Override
  • Steve Fung
Submit Requirements:
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: third_party/glog
Gerrit-Branch: main
Gerrit-Change-Id: I520345493e7047e751c2030081ab42bad95566d3
Gerrit-Change-Number: 1035172
Gerrit-PatchSet: 1
Gerrit-Owner: Leonard Chan <leona...@google.com>
Gerrit-Reviewer: Cameron Dale <camr...@google.com>
Gerrit-Reviewer: Owners Override <owners-...@fuchsia.dev>
Gerrit-Reviewer: Steve Fung <stev...@google.com>
Gerrit-CC: Adam Barth <aba...@google.com>
Gerrit-CC: James Robinson <jam...@google.com>
Gerrit-Attention: Leonard Chan <leona...@google.com>
Gerrit-Attention: Owners Override <owners-...@fuchsia.dev>
Gerrit-Attention: Steve Fung <stev...@google.com>
Gerrit-Attention: Cameron Dale <camr...@google.com>
Gerrit-Comment-Date: Wed, 24 Apr 2024 19:02:24 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No

--
You received this message because you are subscribed to the Google Groups "owners-override" group.
To unsubscribe from this group and stop receiving emails from it, send an email to owners-overri...@fuchsia.dev.
To view this discussion on the web visit https://groups.google.com/a/fuchsia.dev/d/msgid/owners-override/ccf4411aef398e60123d0060679142f5e052f9e2-EmailReviewComments-HTML%40fuchsia-review.googlesource.com.
unsatisfied_requirement
open
diffy

'Cameron Dale (Gerrit)' via owners-override

unread,
Apr 25, 2024, 2:35:14 PMApr 25
to Leonard Chan, James Robinson, Adam Barth, Owners Override, Steve Fung
Attention needed from Leonard Chan, Owners Override and Steve Fung

Cameron Dale voted and added 1 comment

Votes added by Cameron Dale

Code-Review+2

1 comment

Patchset-level comments
Leonard Chan . resolved

Is there an autoroller for glog? Or does it need to be rolled manually?

Cameron Dale

It is rolled manually.

Open in Gerrit

Related details

Attention is currently required from:
  • Leonard Chan
  • Owners Override
  • Steve Fung
Submit Requirements:
  • requirement satisfiedCode-Review
  • requirement satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: third_party/glog
Gerrit-Branch: main
Gerrit-Change-Id: I520345493e7047e751c2030081ab42bad95566d3
Gerrit-Change-Number: 1035172
Gerrit-PatchSet: 1
Gerrit-Owner: Leonard Chan <leona...@google.com>
Gerrit-Reviewer: Cameron Dale <camr...@google.com>
Gerrit-Reviewer: Owners Override <owners-...@fuchsia.dev>
Gerrit-Reviewer: Steve Fung <stev...@google.com>
Gerrit-CC: Adam Barth <aba...@google.com>
Gerrit-CC: James Robinson <jam...@google.com>
Gerrit-Attention: Leonard Chan <leona...@google.com>
Gerrit-Attention: Owners Override <owners-...@fuchsia.dev>
Gerrit-Attention: Steve Fung <stev...@google.com>
Gerrit-Comment-Date: Thu, 25 Apr 2024 18:35:08 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Leonard Chan <leona...@google.com>

--
You received this message because you are subscribed to the Google Groups "owners-override" group.
To unsubscribe from this group and stop receiving emails from it, send an email to owners-overri...@fuchsia.dev.
To view this discussion on the web visit https://groups.google.com/a/fuchsia.dev/d/msgid/owners-override/74f290fb6991f7923ff761f7227c22744b232c24-EmailReviewComments-HTML%40fuchsia-review.googlesource.com.
satisfied_requirement
open
diffy

'Leonard Chan (Gerrit)' via owners-override

unread,
Apr 25, 2024, 2:35:55 PMApr 25
to Cameron Dale, James Robinson, Adam Barth, Owners Override, Steve Fung

Leonard Chan submitted the change

Change information

Commit message:
[glog] Add [[clang::lto_visibility_public]]

With whole program devirtualization enabled, we see a call to
streambuf::underflow incorrectly devirtualized to
basic_streambuf::underflow() when it should instead not be devirtualized
and a vtable call will result in the correct call to
basic_filebuf::underflow. WPD is doing this because we are statically
linking in the definitions and vtables for these classes from libc++.a.
This is an incorrect use of WPD because the definitions of these classes
are outside the LTO unit being linked, so when we attempt to devirt the
calls, the only visible class in the LTO unit is LogStreamBuf thus WPD
assumes the only possible call that can be made for the call to
underflow is the definition from LogStreamBuf, which directly inherits
from std:streambug (aka basic_streambuf).

The correct way to use WPD would be to compile all relevant TUs with
-flto, but we aren't currently able to do that with the llvm runtimes.
Ideally, FatLTO will help with this since we'd ship both bitcode and
objcode, but that's some ways away. For now, what we can do, without
explicitly making the class have default ELF visibility is to make
LogStreamBuf have public LTO visibility. This means the this class is
accessible outside the LTO unit and can have child classes and cannot be
a candidate for devirting.
Bug: 42075686
Change-Id: I520345493e7047e751c2030081ab42bad95566d3
Files:
  • M src/glog/logging.h
Change size: XS
Delta: 1 file changed, 1 insertion(+), 1 deletion(-)
Branch: refs/heads/main
Submit Requirements:
  • requirement satisfiedCode-Review: +2 by Cameron Dale
Open in Gerrit
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: merged
Gerrit-Project: third_party/glog
Gerrit-Branch: main
Gerrit-Change-Id: I520345493e7047e751c2030081ab42bad95566d3
Gerrit-Change-Number: 1035172
Gerrit-PatchSet: 2
Gerrit-Owner: Leonard Chan <leona...@google.com>
Gerrit-Reviewer: Cameron Dale <camr...@google.com>
Gerrit-Reviewer: Leonard Chan <leona...@google.com>
Gerrit-Reviewer: Owners Override <owners-...@fuchsia.dev>
Gerrit-Reviewer: Steve Fung <stev...@google.com>
Gerrit-CC: Adam Barth <aba...@google.com>
Gerrit-CC: James Robinson <jam...@google.com>

--
You received this message because you are subscribed to the Google Groups "owners-override" group.
To unsubscribe from this group and stop receiving emails from it, send an email to owners-overri...@fuchsia.dev.
To view this discussion on the web visit https://groups.google.com/a/fuchsia.dev/d/msgid/owners-override/3259f9a9bedcf79d6255defef7d0bb230dbf1954-HTML%40fuchsia-review.googlesource.com.
open
diffy
satisfied_requirement
Reply all
Reply to author
Forward
0 new messages