New issue
Advanced search Search tips

Issue 847724 link

Starred by 2 users

Issue metadata

Status: Unconfirmed
Owner:
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocked on:
issue 845798



Sign in to add a comment

Enable -fcomplete-member-pointers flag globally

Project Member Reported by p...@chromium.org, May 30 2018

Issue description

This flag was added in LLVM r333498 and it prevents member pointers of incomplete base type from being used in cases where they might cause problems under the Microsoft ABI. It also enables additional semantic analysis that we'll need in order to correctly implement -fsanitize=cfi for member function pointer calls.

I kicked off a build of Chrome for Linux with this flag enabled. So far I've only seen error in one place in swiftshader, and that seems easy to clean up. I also kicked off a Windows cross build, and we'll see how that goes. So it should hopefully be pretty easy to enable globally once we roll clang past the change that added the flag.
 
Project Member

Comment 1 by bugdroid1@chromium.org, May 30 2018

The following revision refers to this bug:
  https://swiftshader.googlesource.com/SwiftShader.git/+/9dfb70f0f2ada3aa690df3f6b5d8e21f296fd6f0

commit 9dfb70f0f2ada3aa690df3f6b5d8e21f296fd6f0
Author: Peter Collingbourne <pcc@google.com>
Date: Wed May 30 20:11:53 2018

Move the declaration of DrawCall after Renderer.

This allows compilers targeting the MS ABI to select the
correct inheritance model for the member function pointer field
DrawCall::setupPrimitives using the complete type of Renderer. It
will allow us to enable the new Clang flag -fcomplete-member-pointers
globally.

Bug: chromium:847724
Change-Id: Ied5859ec2f5d38b3ccf51608527506caec53f470
Reviewed-on: https://swiftshader-review.googlesource.com/19068
Tested-by: Peter Collingbourne <pcc@google.com>
Reviewed-by: Nicolas Capens <nicolascapens@google.com>

[modify] https://crrev.com/9dfb70f0f2ada3aa690df3f6b5d8e21f296fd6f0/src/Renderer/Renderer.hpp

Project Member

Comment 2 by bugdroid1@chromium.org, May 31 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/088233a24965efea5a225c2155200248a328ded8

commit 088233a24965efea5a225c2155200248a328ded8
Author: Peter Collingbourne <pcc@chromium.org>
Date: Thu May 31 00:09:52 2018

Move a template specialization for a member pointer after the declaration of the base class.

This allows compilers targeting the MS ABI to select the correct
inheritance model for the member function pointer type RunnerMethodType
using the complete type of TaskHandle::Runner. It will allow us to
enable the new Clang flag -fcomplete-member-pointers globally.

Bug: 847724
Change-Id: Ib716e3c0774b7d6b46087e46057ca249e9aa72e7
Reviewed-on: https://chromium-review.googlesource.com/1079852
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Peter Collingbourne <pcc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#563078}
[modify] https://crrev.com/088233a24965efea5a225c2155200248a328ded8/third_party/blink/renderer/platform/web_task_runner.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Jun 1 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ee76bddf0afa55b36d9b8fc79d6b5e698e704d7e

commit ee76bddf0afa55b36d9b8fc79d6b5e698e704d7e
Author: Nicolas Capens <capn@chromium.org>
Date: Fri Jun 01 17:37:12 2018

Roll SwiftShader 4b74373..419e8a7

https://swiftshader.googlesource.com/SwiftShader.git/+log/4b74373..419e8a7

BUG=chromium:847724

TBR=kbr@chromium.org

TEST=bots

CQ_INCLUDE_TRYBOTS=luci.chromium.try:win_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_chromium_cfi_rel_ng;luci.chromium.try:android_optional_gpu_tests_rel

Change-Id: I22ac7195358bab6470e2179873747094fd287743
Reviewed-on: https://chromium-review.googlesource.com/1082600
Reviewed-by: Alexis Hétu <sugoi@chromium.org>
Commit-Queue: Nicolas Capens <capn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#563705}
[modify] https://crrev.com/ee76bddf0afa55b36d9b8fc79d6b5e698e704d7e/DEPS

Project Member

Comment 4 by bugdroid1@chromium.org, Jun 13 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3105b4b4381a42d4b426e46a0ccd901582b0f921

commit 3105b4b4381a42d4b426e46a0ccd901582b0f921
Author: Peter Collingbourne <pcc@chromium.org>
Date: Wed Jun 13 20:04:56 2018

gn: Swap the order of two declarations.

This allows compilers targeting the MS ABI to select the correct
inheritance model for the member function pointer types PrefixFunc
and InfixFunc using the complete type of Parser. It will allow us to
enable the new Clang flag -fcomplete-member-pointers globally.

Bug: 847724
Change-Id: I5b3488ce0b64b3ac4884bc499cd355e1651d12b3
Reviewed-on: https://chromium-review.googlesource.com/1098515
Commit-Queue: Peter Collingbourne <pcc@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566964}
[modify] https://crrev.com/3105b4b4381a42d4b426e46a0ccd901582b0f921/tools/gn/parser.h

Project Member

Comment 5 by bugdroid1@chromium.org, Jun 13 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fea64fd2f17f40d572923de0705def0016ad4a9f

commit fea64fd2f17f40d572923de0705def0016ad4a9f
Author: Peter Collingbourne <pcc@chromium.org>
Date: Wed Jun 13 22:17:49 2018

zucchini: Swap the order of two declarations.

This allows compilers targeting the MS ABI to select the correct
inheritance model for the member function pointer types ReaderFactory
and WriterFactory using the complete type of Disassembler. It will
allow us to enable the new Clang flag -fcomplete-member-pointers
globally.

Bug: 847724
Change-Id: Iead5fd97da5f67c336d8904a7a305a34df37065b
Reviewed-on: https://chromium-review.googlesource.com/1098470
Commit-Queue: Peter Collingbourne <pcc@chromium.org>
Reviewed-by: Samuel Huang <huangs@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567016}
[modify] https://crrev.com/fea64fd2f17f40d572923de0705def0016ad4a9f/components/zucchini/disassembler.h

Project Member

Comment 6 by bugdroid1@chromium.org, Jun 14 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4a2a5c4aa89b2718f60d775896784a96068fcacf

commit 4a2a5c4aa89b2718f60d775896784a96068fcacf
Author: Peter Collingbourne <pcc@chromium.org>
Date: Thu Jun 14 01:47:20 2018

build: Pass -fcomplete-member-pointers when building with clang.

This prevents member pointers of incomplete base type from being used in
cases where they might cause problems under the Microsoft ABI.

Specifically, the Microsoft ABI has different kinds of member pointers with
different sizes, and the choice of member pointer representation depends on
the inheritance hierarchy of the member pointer's base type. C++ allows a
member pointer's base type to be incomplete, so if it is incomplete at the
point where a variable of that member pointer type is declared, that forces
the compiler to pick the most general (i.e. largest) one. That can lead to
ODR violations since the most general representation wouldn't necessarily
be the one that would be chosen if the base type happened to be complete at
the point where the variable was declared. It can also be less size efficient
because the compiler will generally be able to choose a smaller representation
than the most general one if it were complete at the point where it is needed.

This flag also enables additional semantic analysis that we'll need in order
to correctly implement -fsanitize=cfi for member function pointer calls. This
is because the inheritance hierarchy of the base type must be available in
order to make the CFI checks as precise as possible.

Note that the flag is a -f flag rather than a -W flag. This is because
requiring member pointer base types to be complete is technically a
non-conforming language extension, as it may, for example, cause templates
to be instantiated which would otherwise not be, which may be observable
after code generation in conforming programs that were crafted to observe
it. However, the effects of this language extension should not be observable
in most ordinary programs.

Bug: 847724
Change-Id: I8d823fd4a6f21dfcadba55eefc0a69ef2e0c3479
Reviewed-on: https://chromium-review.googlesource.com/1098217
Commit-Queue: Peter Collingbourne <pcc@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567086}
[modify] https://crrev.com/4a2a5c4aa89b2718f60d775896784a96068fcacf/build/config/compiler/BUILD.gn

Project Member

Comment 7 by bugdroid1@chromium.org, Jun 14 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/64397686ae3dd0b0eee8b7990e368d6b8a0c5421

commit 64397686ae3dd0b0eee8b7990e368d6b8a0c5421
Author: John Budorick <jbudorick@chromium.org>
Date: Thu Jun 14 02:19:05 2018

Revert "build: Pass -fcomplete-member-pointers when building with clang."

This reverts commit 4a2a5c4aa89b2718f60d775896784a96068fcacf.

Reason for revert: seems to be a bit too new of a flag for the xcode version of clang: https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/ios-simulator-xcode-clang/51865

Original change's description:
> build: Pass -fcomplete-member-pointers when building with clang.
> 
> This prevents member pointers of incomplete base type from being used in
> cases where they might cause problems under the Microsoft ABI.
> 
> Specifically, the Microsoft ABI has different kinds of member pointers with
> different sizes, and the choice of member pointer representation depends on
> the inheritance hierarchy of the member pointer's base type. C++ allows a
> member pointer's base type to be incomplete, so if it is incomplete at the
> point where a variable of that member pointer type is declared, that forces
> the compiler to pick the most general (i.e. largest) one. That can lead to
> ODR violations since the most general representation wouldn't necessarily
> be the one that would be chosen if the base type happened to be complete at
> the point where the variable was declared. It can also be less size efficient
> because the compiler will generally be able to choose a smaller representation
> than the most general one if it were complete at the point where it is needed.
> 
> This flag also enables additional semantic analysis that we'll need in order
> to correctly implement -fsanitize=cfi for member function pointer calls. This
> is because the inheritance hierarchy of the base type must be available in
> order to make the CFI checks as precise as possible.
> 
> Note that the flag is a -f flag rather than a -W flag. This is because
> requiring member pointer base types to be complete is technically a
> non-conforming language extension, as it may, for example, cause templates
> to be instantiated which would otherwise not be, which may be observable
> after code generation in conforming programs that were crafted to observe
> it. However, the effects of this language extension should not be observable
> in most ordinary programs.
> 
> Bug: 847724
> Change-Id: I8d823fd4a6f21dfcadba55eefc0a69ef2e0c3479
> Reviewed-on: https://chromium-review.googlesource.com/1098217
> Commit-Queue: Peter Collingbourne <pcc@chromium.org>
> Reviewed-by: Nico Weber <thakis@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#567086}

TBR=thakis@chromium.org,pcc@chromium.org

Change-Id: Ieb56b1bb4236a874ecc4846eb3ad73a87d9bdf82
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 847724
Reviewed-on: https://chromium-review.googlesource.com/1100516
Reviewed-by: John Budorick <jbudorick@chromium.org>
Commit-Queue: John Budorick <jbudorick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567094}
[modify] https://crrev.com/64397686ae3dd0b0eee8b7990e368d6b8a0c5421/build/config/compiler/BUILD.gn

Project Member

Comment 8 by bugdroid1@chromium.org, Jun 14 2018

The following revision refers to this bug:
  https://pdfium.googlesource.com/pdfium/+/bb3d6313f278feee76f5fba17feff3aee65fb928

commit bb3d6313f278feee76f5fba17feff3aee65fb928
Author: Peter Collingbourne <pcc@google.com>
Date: Thu Jun 14 18:56:08 2018

Move definition of XFA_SCRIPTATTRIBUTEINFO into cjx_object.h.

This allows compilers targeting the MS ABI to select the correct inheritance
model for the member function pointer type XFA_ATTRIBUTE_CALLBACK using the
complete type of CJX_Object. It will allow us to enable the new Clang flag
-fcomplete-member-pointers globally.

Bug: chromium:847724
Change-Id: I90cedde8c5355e5eb896a93f0e43e6a1e1d09dbc
Reviewed-on: https://pdfium-review.googlesource.com/35190
Commit-Queue: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>

[modify] https://crrev.com/bb3d6313f278feee76f5fba17feff3aee65fb928/fxjs/xfa/cjx_object.h
[modify] https://crrev.com/bb3d6313f278feee76f5fba17feff3aee65fb928/xfa/fxfa/fxfa_basic.h

Project Member

Comment 9 by bugdroid1@chromium.org, Jun 15 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5da61dbe887e02c83b99f8ba72e85087f205a397

commit 5da61dbe887e02c83b99f8ba72e85087f205a397
Author: pdfium-chromium-autoroll <pdfium-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Fri Jun 15 00:47:33 2018

Roll src/third_party/pdfium e16ffd4fc3f2..ee3e3a4f3cba (4 commits)

https://pdfium.googlesource.com/pdfium.git/+log/e16ffd4fc3f2..ee3e3a4f3cba


git log e16ffd4fc3f2..ee3e3a4f3cba --date=short --no-merges --format='%ad %ae %s'
2018-06-14 npm@chromium.org Merge 3 methods of CJBig2_GRDProc together
2018-06-14 dsinclair@chromium.org [formcalc] Calculate length of string when calling FXSYS_wcstof
2018-06-14 pcc@google.com Move definition of XFA_SCRIPTATTRIBUTEINFO into cjx_object.h.
2018-06-14 hnakashima@chromium.org Rewrite content stream regeneration.


Created with:
  gclient setdep -r src/third_party/pdfium@ee3e3a4f3cba

The AutoRoll server is located here: https://pdfium-roll.skia.org

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.



BUG= chromium:846104 ,chromium:847724
TBR=dsinclair@chromium.org

Change-Id: Ib391fca7b13757f3b2dc733c4d20fd67d42d1d96
Reviewed-on: https://chromium-review.googlesource.com/1101720
Reviewed-by: pdfium-chromium-autoroll <pdfium-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Commit-Queue: pdfium-chromium-autoroll <pdfium-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#567501}
[modify] https://crrev.com/5da61dbe887e02c83b99f8ba72e85087f205a397/DEPS

Project Member

Comment 10 by bugdroid1@chromium.org, Jun 15 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/10bcc4d1db5c4b506aff7fa9f5e9e0d8226c84c6

commit 10bcc4d1db5c4b506aff7fa9f5e9e0d8226c84c6
Author: Peter Collingbourne <pcc@chromium.org>
Date: Fri Jun 15 00:52:04 2018

Reland "build: Pass -fcomplete-member-pointers when building with clang."

This is a reland of 4a2a5c4aa89b2718f60d775896784a96068fcacf
with an additional check for use_xcode_clang.

Original change's description:
> build: Pass -fcomplete-member-pointers when building with clang.
>
> This prevents member pointers of incomplete base type from being used in
> cases where they might cause problems under the Microsoft ABI.
>
> Specifically, the Microsoft ABI has different kinds of member pointers with
> different sizes, and the choice of member pointer representation depends on
> the inheritance hierarchy of the member pointer's base type. C++ allows a
> member pointer's base type to be incomplete, so if it is incomplete at the
> point where a variable of that member pointer type is declared, that forces
> the compiler to pick the most general (i.e. largest) one. That can lead to
> ODR violations since the most general representation wouldn't necessarily
> be the one that would be chosen if the base type happened to be complete at
> the point where the variable was declared. It can also be less size efficient
> because the compiler will generally be able to choose a smaller representation
> than the most general one if it were complete at the point where it is needed.
>
> This flag also enables additional semantic analysis that we'll need in order
> to correctly implement -fsanitize=cfi for member function pointer calls. This
> is because the inheritance hierarchy of the base type must be available in
> order to make the CFI checks as precise as possible.
>
> Note that the flag is a -f flag rather than a -W flag. This is because
> requiring member pointer base types to be complete is technically a
> non-conforming language extension, as it may, for example, cause templates
> to be instantiated which would otherwise not be, which may be observable
> after code generation in conforming programs that were crafted to observe
> it. However, the effects of this language extension should not be observable
> in most ordinary programs.
>
> Bug: 847724
> Change-Id: I8d823fd4a6f21dfcadba55eefc0a69ef2e0c3479
> Reviewed-on: https://chromium-review.googlesource.com/1098217
> Commit-Queue: Peter Collingbourne <pcc@chromium.org>
> Reviewed-by: Nico Weber <thakis@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#567086}

Bug: 847724
Change-Id: I1b97980691914492945d170931d33438c68e8d0b
Reviewed-on: https://chromium-review.googlesource.com/1101477
Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Peter Collingbourne <pcc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567505}
[modify] https://crrev.com/10bcc4d1db5c4b506aff7fa9f5e9e0d8226c84c6/build/config/compiler/BUILD.gn

Is anything required to update development environments to use this flag? After a git pull today, I get many errors like the following:

clang: error: unknown argument: '-fcomplete-member-pointers'
Did you run `gclient sync`? What's `third_party/llvm-build/Release+Asserts/bin/clang --version` for you?
As mentioned here [https://groups.google.com/forum/#!topic/discuss-webrtc/-8eyqp6qw0A] that flag causes compiler failure on Linux if use_custom_libcxx is set to false to use the host's libstdc++. It's failing with the current latest libstdc++ 6.0.25. Appending another conditional to check for use_custom_libcxx fixes it.

Are you synced past https://chromium-review.googlesource.com/c/chromium/src/+/1114848 ? If not, that'll fix it.
I'm building webrtc and it doesn't look it picked that up just yet... But applying that diff manually does fix it in the interim. Thanks.

Sign in to add a comment