Groups keyboard shortcuts have been updated
Dismiss
See shortcuts
[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
Apr 24, 2024, 2:28:39 PM4/24/24
to Owners Override
Attention needed from Cameron Dale, Owners Override and Steve Fung
Leonard Chan has uploaded the change for review![]( "Open in Gerrit")
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(-)
Related details
Attention is currently required from:
- Cameron Dale
- Owners Override
- Steve Fung
Submit Requirements:
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
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.
'Adam Barth (Gerrit)' via owners-override
Apr 24, 2024, 3:01:02 PM4/24/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![]( "Open in Gerrit")
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.
Related details
Attention is currently required from:
- Cameron Dale
- Leonard Chan
- Owners Override
- Steve Fung
Submit Requirements:
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
--
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.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.
'James Robinson (Gerrit)' via owners-override
Apr 24, 2024, 3:02:30 PM4/24/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![]( "Open in Gerrit")
Patchset-level comments
James Robinson . resolved
I don't think we have owners checking enabled in this repository at all.
Related details
Attention is currently required from:
- Cameron Dale
- Leonard Chan
- Owners Override
- Steve Fung
Submit Requirements:
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
--
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.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.
'Cameron Dale (Gerrit)' via owners-override
Apr 25, 2024, 2:35:14 PM4/25/24
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![]( "Open in Gerrit")
Votes added by Cameron Dale
| Code-Review | +2 |
1 comment
Patchset-level comments
Leonard Chan . resolved
Cameron DaleIs there an autoroller for glog? Or does it need to be rolled manually?
It is rolled manually.
Related details
Attention is currently required from:
- Leonard Chan
- Owners Override
- Steve Fung
Submit Requirements:
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
--
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.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.
'Leonard Chan (Gerrit)' via owners-override
Apr 25, 2024, 2:35:55 PM4/25/24
to Cameron Dale, James Robinson, Adam Barth, Owners Override, Steve Fung
Leonard Chan submitted the change![]( "Open in Gerrit")
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
Reviewed-by: Cameron Dale <camr...@google.com>
Files:
- M src/glog/logging.h
Change size: XS
Delta: 1 file changed, 1 insertion(+), 1 deletion(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +2 by Cameron Dale
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
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.To unsubscribe from this group and stop receiving emails from it, send an email to owners-overri...@fuchsia.dev.
Reply all
Reply to author
Forward
0 new messages