Comment

In D128492#3606984, @MaskRay wrote:

Thanks for catching this case. For absolute symbols we can choose not relaxing in -no-pie mode. Do you prefer relaxing?

Yes, but not indirectly called from C/C++ but rather from compiler-generated code right? That's presumably why this patch + D116130 worked for @zhuhan0.

LGTM

LGTM

Okay, it seems reasonable enough to have the [[clang::lto_visibility_public]] attribute override the --lto-whole-program-visibility flag.

I don't think I would expect the stage 2 runtimes to be built with a sanitizer even if use_asan is set. The sanitizer runtimes themselves don't support being built with sanitizers, and they might be compiled to depend on the builtins, which would create a circular dependency. So I think use_asan and such should only affect the stage 1 compiler. I think I would favor adding use_asan = false to the stage2_unix_toolchain template.

In D127876#3586154, @aeubanks wrote:

In D127876#3586134, @pcc wrote:

This diverges from the documented behavior of -lto-whole-program-visibility. The flag is meant to give all classes hidden LTO visibility, but now it only does that to some of them.

perhaps I'm misunderstanding, but even with -lto-whole-program-visibility classes explicitly marked with public LTO visibility still shouldn't participate in WPD?

This diverges from the documented behavior of -lto-whole-program-visibility. The flag is meant to give all classes hidden LTO visibility, but now it only does that to some of them.

In D126089#3583129, @aeubanks wrote:

In D126089#3582321, @pcc wrote:

In D126089#3582227, @aeubanks wrote:

is this patch ok to submit at least as a short term workaround?

Chrome already has a short-term workaround, which is to disable opaque pointers, right? I think you can use that until the proper fix is developed.

As upstream tests move away from testing typed pointers, I'm worried that pinning to typed pointers will cause regressions that we don't want to spend time investigating. I'm not sure how long a redesign/rewrite would take so I'd like to get Chrome off of typed pointers soonish.

In D126089#3582568, @tejohnson wrote:

In D126089#3582321, @pcc wrote:

In D126089#3582227, @aeubanks wrote:

is this patch ok to submit at least as a short term workaround?

Chrome already has a short-term workaround, which is to disable opaque pointers, right? I think you can use that until the proper fix is developed.

As noted, I'm really not convinced that we cannot hit this issue for hidden visibility patches.

However, another reason to submit this particular patch is that it is merely enabling WPD in additional cases, when there is a phi between the type test and assume, by recognizing more assumes. I would imagine we will still have cases like that even for hidden visibility classes, or with your proposed fix to use a new TBD intrinsic that is converted to an llvm.type.test in the case of --lto-whole-program-visibility, right? So in either case this seems to me to be an improvement.

In D126089#3582227, @aeubanks wrote:

is this patch ok to submit at least as a short term workaround?

Fix Mac build

Stop sync_source_lists_from_cmake.py from finding the reference to the source file

In D126089#3571368, @tejohnson wrote:

In D126089#3571208, @pcc wrote:

In D126089#3570116, @tejohnson wrote:

In D126089#3569012, @pcc wrote:

Thanks for the reproducer!

I think the root cause of the issue here is that the IR generated by Clang is wrong. llvm.type.test will only consider types that are local to the LTO unit, which means that it is incorrect to emit llvm.assume(llvm.type.test) for types A or B, because they have public LTO visibility, which means that definitions may reside outside of the LTO unit. The LowerTypeTests pass is therefore correctly replacing the llvm.type.tests with false, leading to the miscompilation seen here. The issue can also be seen if the [[clang::lto_visibility_public]] attribute is removed.

This is by design per the changes I made awhile back to delay the decision about visibility until LTO link time: https://groups.google.com/g/llvm-dev/c/6LfIiAo9g68/m/Ng1Xvw4WAwAJ
They should have public !vcall_visibility metadata by default, and since whole program visibility isn't being asserted here, there should be no devirt, etc.

The problem with this design is that it leaves the meaning of llvm.type.test undefined in the case where there are no globals that are a member of the type, because the !vcall_visibility is associated with the global. So there's no way to know whether the type argument was intended to refer to a type with hidden LTO visibility or one with public LTO visibility, if there are no member globals.

Since -lto-whole-program-visibility is a flag that affects the operation of the type tests globally, I think a better design would be to replace all llvm.type.test calls with true, and replaces llvm.type.checked.load with a regular load, if the flag is not passed.

We only perform WPD if there is a global, however. And then we subsequently remove these sequences if there is no global for a type id [1].

Maybe I am misunderstanding the concern though, can you give an example where this will go wrong?

In D126089#3571365, @aeubanks wrote:

Since -lto-whole-program-visibility is a flag that affects the operation of the type tests globally, I think a better design would be to replace all llvm.type.test calls with true, and replaces llvm.type.checked.load with a regular load, if the flag is not passed.

My original patch (https://reviews.llvm.org/D126089?id=431037) replaced llvm.type.test with true in all cases, but tejohnson said

Regarding the quoted comment, note it is "type test assume" sequences, not all type tests, that need to be removed here in certain circumstances. Note also that before I made the change to try to keep these around longer for use by other analyses, this code attempted to remove all "type test assume" sequences. I.e. it removed all the analyzed assumes, and any type test that was subsequently dead. Presumably that version of WPD+LTT would have also illustrated the original problem this patch tries to address (an assume fed by a phi, in turn fed by a type test that got an Unsat and resulted in llvm.assume(false)), and for a WPD+CFI build we certainly wouldn't want to replace *all* type test uses in the code at this point with true. So I am skeptical that it is correct to replace type test non-assume (non-phi) uses with true in the more limited cases being handled currently either.

In D126089#3570116, @tejohnson wrote:

In D126089#3569012, @pcc wrote:

Thanks for the reproducer!

I think the root cause of the issue here is that the IR generated by Clang is wrong. llvm.type.test will only consider types that are local to the LTO unit, which means that it is incorrect to emit llvm.assume(llvm.type.test) for types A or B, because they have public LTO visibility, which means that definitions may reside outside of the LTO unit. The LowerTypeTests pass is therefore correctly replacing the llvm.type.tests with false, leading to the miscompilation seen here. The issue can also be seen if the [[clang::lto_visibility_public]] attribute is removed.

This is by design per the changes I made awhile back to delay the decision about visibility until LTO link time: https://groups.google.com/g/llvm-dev/c/6LfIiAo9g68/m/Ng1Xvw4WAwAJ
They should have public !vcall_visibility metadata by default, and since whole program visibility isn't being asserted here, there should be no devirt, etc.

Thanks for the reproducer!

Doesn't it also need to be removed from the list in llvm/lib/Target/AArch64/AArch64.td?

In D126089#3538950, @aeubanks wrote:

In D126089#3538842, @pcc wrote:

In D126089#3538826, @aeubanks wrote:

In D126089#3538760, @pcc wrote:

This case is supposed to be handled as a result of running the WPD pass early. See the comment near the top of PassBuilder::buildThinLTODefaultPipeline:

// These passes import type identifier resolutions for whole-program
// devirtualization and CFI. They must run early because other passes may
// disturb the specific instruction patterns that these passes look for,
// creating dependencies on resolutions that may not appear in the summary.
//
// For example, GVN may transform the pattern assume(type.test) appearing in
// two basic blocks into assume(phi(type.test, type.test)), which would
// transform a dependency on a WPD resolution into a dependency on a type
// identifier resolution for CFI.
//
// Also, WPD has access to more precise information than ICP and can
// devirtualize more effectively, so it should operate on the IR first.
//
// The WPD and LowerTypeTest passes need to run at -O0 to lower type
// metadata and intrinsics.

Is there some way that we can now end up with GVN or some other such pass running between when we create the summary and when we run WPD that's inserting a phi?

I thought the frontend emitted the type test/assume. I'm seeing them in the IR after the pre link step. CodeGenFunction::EmitTypeMetadataCodeForVCall() in clang emits a type test/assume.

Right, but the phi must be coming from somewhere else after summary generation if it isn't accounted for in the summary (if the phi was inserted before summary generation, the summary should end up with a dependency on a CFI type identifier resolution, instead of a WPD resolution). Can you trace where it is coming from?

I'm not super familiar with ThinLTO summaries, could you explain a bit more?
The assume(phi(type test, type test)) is coming from the output of the pre link. Then post link WPD looks at all type test usages and I think LTT does the same? So not sure where the summary is relevant.

In D126089#3538826, @aeubanks wrote:

In D126089#3538760, @pcc wrote:

This case is supposed to be handled as a result of running the WPD pass early. See the comment near the top of PassBuilder::buildThinLTODefaultPipeline:

// These passes import type identifier resolutions for whole-program
// devirtualization and CFI. They must run early because other passes may
// disturb the specific instruction patterns that these passes look for,
// creating dependencies on resolutions that may not appear in the summary.
//
// For example, GVN may transform the pattern assume(type.test) appearing in
// two basic blocks into assume(phi(type.test, type.test)), which would
// transform a dependency on a WPD resolution into a dependency on a type
// identifier resolution for CFI.
//
// Also, WPD has access to more precise information than ICP and can
// devirtualize more effectively, so it should operate on the IR first.
//
// The WPD and LowerTypeTest passes need to run at -O0 to lower type
// metadata and intrinsics.

Is there some way that we can now end up with GVN or some other such pass running between when we create the summary and when we run WPD that's inserting a phi?

I thought the frontend emitted the type test/assume. I'm seeing them in the IR after the pre link step. CodeGenFunction::EmitTypeMetadataCodeForVCall() in clang emits a type test/assume.

This case is supposed to be handled as a result of running the WPD pass early. See the comment near the top of PassBuilder::buildThinLTODefaultPipeline:

// These passes import type identifier resolutions for whole-program
// devirtualization and CFI. They must run early because other passes may
// disturb the specific instruction patterns that these passes look for,
// creating dependencies on resolutions that may not appear in the summary.
//
// For example, GVN may transform the pattern assume(type.test) appearing in
// two basic blocks into assume(phi(type.test, type.test)), which would
// transform a dependency on a WPD resolution into a dependency on a type
// identifier resolution for CFI.
//
// Also, WPD has access to more precise information than ICP and can
// devirtualize more effectively, so it should operate on the IR first.
//
// The WPD and LowerTypeTest passes need to run at -O0 to lower type
// metadata and intrinsics.

Is there some way that we can now end up with GVN or some other such pass running between when we create the summary and when we run WPD that's inserting a phi?

In D120727#3512177, @paulkirth wrote:

Hi, we're seeing some failures in Fuchsia's Clang CI. Our runtimes build seems to be unable to find cxxabi.h.

The failing bot can be found here: https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8814278370664903633/overview

Ping.

In D119296#3467905, @samitolvanen wrote:

In D119296#3466027, @joaomoreira wrote:

I played a little bit with kcfi and here are some thoughts:

• under -Os I saw functions being inlined, regardless of the source code calling them indirectly. In these scenarios, the KCFI check was still in place, even though there was not a pointer involved in the call. Although not semantically incorrect, it would be great to prevent the unnecessary overhead (see attached source/compile it with -Os and -fsanitize=kcfi).

Yes, I suspect this might be an issue with Clang's existing CFI schemes too, and would probably require an additional pass to drop checks before calls that were either inlined or optimized into direct calls.

LGTM

I think this will introduce a correctness issue as it will become possible for two modules that can be linked together to have the same unique module ID.

LGTM but please add a test for the autoupgrader.

Can we add logic to llvm/lib/IR/AutoUpgrade.cpp to change Error to Min if we see one of these module flags? I think that should allow old bitcode to continue working.

Note that if additional data has been injected between the KCFI
type identifier and the start of the function, e.g. by using
-fpatchable-function-entry, the offset in bytes must be specified
using -fsanitize-kcfi-offset=<value> to avoid errors. The offset
must be the same for all indirectly called functions in every
translation unit.

In D122862#3433839, @thakis wrote:

This broke building on non-linux: http://45.33.8.238/macm1/32578/step_4.txt

Ptal and revert if it takes a while to fix.

IIRC 1 << 21 was from COFF (missed that the addend was signed), so it wouldn't make sense to have this be 1 << 21 on other object formats if the actual limit is higher.

In D85649#2281439, @chill wrote:

It looks like the only value that makes sense is Error - any other policy (existing or not) would potentially lead to meaningfully different code generated with or without LTO.

LGTM

Use isARM() etc

In D115844#3321235, @ychen wrote:

In D115844#3321190, @pcc wrote:

On the bug you have:

define internal fastcc void @​_Z4callIiE4taskv.resume(%_Z4callIiE4taskv.Frame* noalias nonnull align 8 dereferenceable(24
    ) %FramePtr) #&#8203;1 prologue <{ i32, i32 }> <{ i32 846595819, i32 trunc (i64 sub (i64 ptrtoint (i8** @&#8203;1 to i64), i64 ptrtoint (void ()* @&#8203;_Z4callIiE4taskv to i64)) to i32) }> {...}

Is it possible for the C/C++ code to take the address of the function _Z4callIiE4taskv.resume and call it indirectly?

*.resume is a compiler inserted function that is opaque to the programmer. It is called indirectly most of the time if not all the time.

On the bug you have:

define internal fastcc void @​_Z4callIiE4taskv.resume(%_Z4callIiE4taskv.Frame* noalias nonnull align 8 dereferenceable(24
    ) %FramePtr) #&#8203;1 prologue <{ i32, i32 }> <{ i32 846595819, i32 trunc (i64 sub (i64 ptrtoint (i8** @&#8203;1 to i64), i64 ptrtoint (void ()* @&#8203;_Z4callIiE4taskv to i64)) to i32) }> {...}

Is it possible for the C/C++ code to take the address of the function _Z4callIiE4taskv.resume and call it indirectly? If not, it seems like the right fix would be to arrange for the prologue data to be dropped on the .resume function instead of duplicating it there. I would also imagine that whatever signature you have on the .resume function would be incorrect since it appears that the coro splitting pass will use a different function signature for that function.

I suspect the real issue is that we're missing #include <stddef.h> at the top of this file. What happens if you add that and don't std qualify everything?

In addition to the .bc support we will also need support for reading/writing .ll files.

Make it conditional