Page MenuHomePhabricator

tejohnson (Teresa Johnson)
User

Projects

User does not belong to any projects.

User Details

User Since
Apr 27 2015, 11:17 AM (299 w, 4 d)

Recent Activity

Fri, Jan 15

tejohnson accepted D94783: [LTO] Remove options to disable inlining and GVNLoadPRE..

lgtm, thanks!

Fri, Jan 15, 9:55 AM · Restricted Project
tejohnson added inline comments to D94783: [LTO] Remove options to disable inlining and GVNLoadPRE..
Fri, Jan 15, 9:32 AM · Restricted Project
tejohnson added a comment to D94783: [LTO] Remove options to disable inlining and GVNLoadPRE..

Agree, these should just be removed. but I also don't see any uses of disable-lto-vectorization in a test, can that and the corresponding DisableVectorization parameter be nuked as well?

Fri, Jan 15, 8:57 AM · Restricted Project

Thu, Jan 14

tejohnson added inline comments to D91583: [LTO] Prevent devirtualization for symbols exported to dynamic linker.
Thu, Jan 14, 3:12 PM · Restricted Project
tejohnson updated the diff for D91583: [LTO] Prevent devirtualization for symbols exported to dynamic linker.

Use linkonce_odr vtables to illustrate issue with Symbol exportDynamic

Thu, Jan 14, 3:12 PM · Restricted Project
tejohnson added inline comments to D91583: [LTO] Prevent devirtualization for symbols exported to dynamic linker.
Thu, Jan 14, 3:07 PM · Restricted Project
tejohnson added inline comments to D91583: [LTO] Prevent devirtualization for symbols exported to dynamic linker.
Thu, Jan 14, 2:18 PM · Restricted Project
tejohnson updated the diff for D91583: [LTO] Prevent devirtualization for symbols exported to dynamic linker.

Rebase and use ";;" instead of ";" for comments.

Thu, Jan 14, 2:16 PM · Restricted Project
tejohnson committed rG5b42fd8dd4e7: [LTO] Test format fix (NFC) (authored by tejohnson).
[LTO] Test format fix (NFC)
Thu, Jan 14, 2:11 PM
tejohnson accepted D94655: [Driver] -gsplit-dwarf: Produce .dwo regardless of -gN for IR input.

lgtm

Thu, Jan 14, 8:28 AM · Restricted Project

Wed, Jan 13

tejohnson added inline comments to D94647: [Driver] -gsplit-dwarf: Produce .dwo regardless of -gN for -fthinlto-index=.
Wed, Jan 13, 11:08 PM · Restricted Project
tejohnson added inline comments to D94647: [Driver] -gsplit-dwarf: Produce .dwo regardless of -gN for -fthinlto-index=.
Wed, Jan 13, 10:44 PM · Restricted Project

Tue, Jan 12

tejohnson updated the diff for D91583: [LTO] Prevent devirtualization for symbols exported to dynamic linker.

Address comments

Tue, Jan 12, 3:53 PM · Restricted Project
tejohnson added inline comments to D91583: [LTO] Prevent devirtualization for symbols exported to dynamic linker.
Tue, Jan 12, 3:52 PM · Restricted Project
tejohnson added inline comments to D91583: [LTO] Prevent devirtualization for symbols exported to dynamic linker.
Tue, Jan 12, 3:05 PM · Restricted Project
tejohnson added a comment to D94487: [LTO] Use lto::backend for code generation (WIP)..

Haven't looked at the detailed changes yet, but a couple of high level comments/questions below.

Tue, Jan 12, 8:28 AM · Restricted Project
tejohnson added a comment to D94487: [LTO] Use lto::backend for code generation (WIP)..

Thanks, I'll take a look hopefully today. As I just mentioned in the bug, ThinLTOCodeGenerator has the same issue - will you be migrating that as well?

Tue, Jan 12, 7:54 AM · Restricted Project

Thu, Jan 7

tejohnson added a comment to D91583: [LTO] Prevent devirtualization for symbols exported to dynamic linker.

I implemented @MaskRay's suggestion and added a bit to convey whether a symbol is exported to the dynamic linker (via --export-dynamic[-symbol=] or --dynamic-list=), and use that to prevent the LTO visibility upgrade for WPD. I added support to both lld and gold plugin, and associated tests. Note that I couldn't use includeInDynsym in lld because that is not set for linkonce_odr symbols that were thus have canBeOmittedFromSymbolTable set (since any referencing module must have it's own copy) - we still want to block the LTO visibility upgrade for those symbols to avoid WPD. So I am using a slightly different interface that more directly checks whether export-dynamic is in effect.

Thu, Jan 7, 9:21 AM · Restricted Project

Thu, Dec 31

tejohnson accepted D93959: [ThinLTO] Default -enable-import-metadata to false.

LGTM, I've been bitten by this in new tests as well. We can enable it when needed.

Thu, Dec 31, 9:43 AM · Restricted Project

Wed, Dec 30

tejohnson accepted D92899: [ThinLTO][test] Add visibility related tests.

lgtm thanks!

Wed, Dec 30, 4:48 PM · Restricted Project
tejohnson added a comment to D92899: [ThinLTO][test] Add visibility related tests.

Couple minor suggestions copied from review for D92900. Also, I think the macho test will fail right now since it references some symbols that don't exist there?

Wed, Dec 30, 4:18 PM · Restricted Project
tejohnson retitled D91583: [LTO] Prevent devirtualization for symbols exported to dynamic linker from [lld] Allow --export-dynamic to override --lto-whole-program-visibility to [LTO] Prevent devirtualization for symbols exported to dynamic linker.
Wed, Dec 30, 4:03 PM · Restricted Project
tejohnson updated the diff for D91583: [LTO] Prevent devirtualization for symbols exported to dynamic linker.

Update per previous comment

Wed, Dec 30, 4:02 PM · Restricted Project
tejohnson added a comment to D91583: [LTO] Prevent devirtualization for symbols exported to dynamic linker.

I implemented @MaskRay's suggestion and added a bit to convey whether a symbol is exported to the dynamic linker (via --export-dynamic[-symbol=] or --dynamic-list=), and use that to prevent the LTO visibility upgrade for WPD. I added support to both lld and gold plugin, and associated tests. Note that I couldn't use includeInDynsym in lld because that is not set for linkonce_odr symbols that were thus have canBeOmittedFromSymbolTable set (since any referencing module must have it's own copy) - we still want to block the LTO visibility upgrade for those symbols to avoid WPD. So I am using a slightly different interface that more directly checks whether export-dynamic is in effect.

Wed, Dec 30, 4:00 PM · Restricted Project

Dec 21 2020

tejohnson committed rZORGfdb10180aeb7: [buildbot] Force lld for ThinLTO whole program devirtualization worker (authored by tejohnson).
[buildbot] Force lld for ThinLTO whole program devirtualization worker
Dec 21 2020, 10:47 AM
tejohnson closed D93651: [buildbot] Force lld for ThinLTO whole program devirtualization worker.
Dec 21 2020, 10:47 AM
tejohnson requested review of D93651: [buildbot] Force lld for ThinLTO whole program devirtualization worker.
Dec 21 2020, 10:34 AM

Dec 17 2020

tejohnson added a comment to D73418: [WPD] Emit vcall_visibility metadata for MicrosoftCXXABI.

@tejohnson I was wondering where does this @anon.{{.*}} gets set, or in general where can I find more information about it.

When I step through the code in emitVTableTypeMetadata, it's still.

@0 = private unnamed_addr constant { [2 x i8*] } { [2 x i8*] [i8* bitcast (%rtti.CompleteObjectLocator* @"??_R4A@?A0x3E47408C@@6B@" to i8*), i8* bitcast (i32 (%"struct.(anonymous namespace)::A"*)* @"?f@A@?A0x3E47408C@@UEAAHXZ" to i8*)] }, comdat($"??_7A@?A0x3E47408C@@6B@")

Thanks

Dec 17 2020, 7:08 PM · Restricted Project

Dec 16 2020

tejohnson added a comment to D90782: [LTO] set data layout for module before optimize&codegen.

Sorry, completely missed this earlier. Is this fixing a bug? What is the effect on behavior?

Dec 16 2020, 10:27 AM · Restricted Project

Dec 9 2020

tejohnson added a comment to D92900: [ThinLTO] Add Visibility bits to GlobalValueSummary::GVFlags.

From your patch description:

Imported functions and variables get the visibility from the prevailing module.

Dec 9 2020, 11:35 PM · Restricted Project
tejohnson added a comment to D91474: [buildbot] Fix worker for ThinLTO whole program devirtualization.

Point your worker to the port 9994, instead of 9990. Everything is the same
as for the production bot.

This is described in https://llvm.org/docs/HowToAddABuilder.html but I
guess that's buried in other details.

Dec 9 2020, 9:19 PM
tejohnson added inline comments to D92900: [ThinLTO] Add Visibility bits to GlobalValueSummary::GVFlags.
Dec 9 2020, 1:59 PM · Restricted Project
tejohnson accepted D92916: [LLD][gold] Add -plugin-opt=no-new-pass-manager.

LGTM with a minor fix: Patch description has a typo, mentions -fno-pass-manager, I think this should be -fno-new-pass-manager?

Dec 9 2020, 12:51 PM · Restricted Project
tejohnson added a comment to D91474: [buildbot] Fix worker for ThinLTO whole program devirtualization.

Hi Teresa,

LGTM with a nit pick. Please see my comment inline.

if there is a way to test the LTO builder change?

Once the patch is ready we will apply it to the staging. Then you could test it by moving your bot there.

Dec 9 2020, 8:37 AM
tejohnson committed rZORG7342c1f39cfa: [buildbot] Fix worker for ThinLTO whole program devirtualization (authored by tejohnson).
[buildbot] Fix worker for ThinLTO whole program devirtualization
Dec 9 2020, 8:36 AM
tejohnson closed D91474: [buildbot] Fix worker for ThinLTO whole program devirtualization.
Dec 9 2020, 8:36 AM
tejohnson updated the diff for D91474: [buildbot] Fix worker for ThinLTO whole program devirtualization.

Implement suggestion

Dec 9 2020, 8:35 AM

Dec 8 2020

tejohnson accepted D92870: [llvm-lto2] Use NPM with ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER.

lgtm

Dec 8 2020, 10:50 AM · Restricted Project
tejohnson committed rGd7e71b5db842: [compiler-rt santizer] Use clock_gettime instead of timespec_get (authored by jeroen.dobbelaere).
[compiler-rt santizer] Use clock_gettime instead of timespec_get
Dec 8 2020, 10:11 AM
tejohnson closed D91687: [compiler-rt santizer] Use clock_gettime instead of timespec_get.
Dec 8 2020, 10:10 AM · Restricted Project
tejohnson added a comment to D91687: [compiler-rt santizer] Use clock_gettime instead of timespec_get.

I'm having some new issues committing at the moment that I can't figure out (just emailed llvm-dev). @vitalybuka can you commit this?

Maybe they are caused by the change in the branch names ('master' -> 'main') ?

Dec 8 2020, 7:50 AM · Restricted Project
tejohnson committed rG77b509710ce7: [ICP] Don't promote when target not defined in module (authored by tejohnson).
[ICP] Don't promote when target not defined in module
Dec 8 2020, 7:46 AM
tejohnson closed D92804: [ICP] Don't promote when target not defined in module.
Dec 8 2020, 7:45 AM · Restricted Project
tejohnson added a comment to D91687: [compiler-rt santizer] Use clock_gettime instead of timespec_get.

Could you commit this for me ? I don't have write access. Thanks !

Dec 8 2020, 7:40 AM · Restricted Project

Dec 7 2020

tejohnson abandoned D92673: [ThinLTO] Remove unused symbol declarations when possible.

Subsumed by D92804

Dec 7 2020, 5:18 PM · Restricted Project, Restricted Project
tejohnson requested review of D92804: [ICP] Don't promote when target not defined in module.
Dec 7 2020, 5:16 PM · Restricted Project

Dec 4 2020

tejohnson requested review of D92673: [ThinLTO] Remove unused symbol declarations when possible.
Dec 4 2020, 11:16 AM · Restricted Project, Restricted Project

Dec 3 2020

tejohnson accepted D91687: [compiler-rt santizer] Use clock_gettime instead of timespec_get.

lgtm

Dec 3 2020, 7:46 AM · Restricted Project

Dec 2 2020

tejohnson added a comment to D91474: [buildbot] Fix worker for ThinLTO whole program devirtualization.

Ping - @gkistanova can you take a look and also let me know if there is a way to test the LTO builder change?

Dec 2 2020, 8:02 AM

Dec 1 2020

tejohnson accepted D92335: [ThinLTO] Import symver directives for imported symbols (PR48214).

lgtm. Thanks for fixing this, and that's a nice cleanup on the existing handling! One request - could you submit the change to the current handling and associated test changes first, with the symver handling/test as a follow on?

Dec 1 2020, 12:44 PM · Restricted Project
tejohnson added inline comments to D92335: [ThinLTO] Import symver directives for imported symbols (PR48214).
Dec 1 2020, 11:22 AM · Restricted Project
tejohnson added a comment to D92335: [ThinLTO] Import symver directives for imported symbols (PR48214).

I just realized this makes Linker/ depend on Object/
I'm not sure whether that's a problem or not, but we could avoid that by storing the symvers in a metadata node again.

Dec 1 2020, 8:38 AM · Restricted Project

Nov 30 2020

tejohnson added a comment to D92335: [ThinLTO] Import symver directives for imported symbols (PR48214).

Thinking through this some more, I think I was making it too hard. I think we should just be able to collect the symvers out of the inline asm when we do the IR linking. However, not in the place you've modified below, as I note there. Instead, take a look at IRLinker::run(), where it appends the module inline asm strings together if !IsPerformingImport. I think here you could add a case where IsPerformingImport, and just invoke ModuleSymbolTable::CollectAsmSymvers here and directly add any symver you find to the DstM (probably do all this in a helper function). I was thinking that we might need to carry it via module metadata to avoid parsing it during IR linking, but I'm now not sure that's necessary - I don't think it should add significant overhead. Let me know if you run into issues with that approach.

Nov 30 2020, 9:50 PM · Restricted Project
tejohnson accepted D85808: [Remarks][2/2] Expand remarks hotness threshold option support in more tools.

lgtm with a couple of minor nits noted below that you can fix before submitting

Nov 30 2020, 8:20 PM · Restricted Project, Restricted Project
tejohnson accepted D85809: [Remarks][1/2] Expand remarks hotness threshold option support in more tools.

lgtm

Nov 30 2020, 8:20 PM · Restricted Project

Nov 24 2020

tejohnson committed rG07f234be1ccb: [lld] Add --no-lto-whole-program-visibility (authored by tejohnson).
[lld] Add --no-lto-whole-program-visibility
Nov 24 2020, 4:46 PM
tejohnson closed D92060: [lld] Add --no-lto-whole-program-visibility.
Nov 24 2020, 4:46 PM · Restricted Project
tejohnson updated the diff for D92060: [lld] Add --no-lto-whole-program-visibility.

Address comments

Nov 24 2020, 3:12 PM · Restricted Project
tejohnson added inline comments to D92060: [lld] Add --no-lto-whole-program-visibility.
Nov 24 2020, 3:07 PM · Restricted Project
tejohnson added a comment to D91583: [LTO] Prevent devirtualization for symbols exported to dynamic linker.

While I investigate alternative mechanisms to handle --export-dynamic, I've sent D92060 to add a --no- version of the option, to simplify working around issues when --lto-whole-program-visibility is specified broadly.

Nov 24 2020, 2:57 PM · Restricted Project
tejohnson requested review of D92060: [lld] Add --no-lto-whole-program-visibility.
Nov 24 2020, 2:56 PM · Restricted Project
tejohnson committed rG0768b0576a93: Avoid redundant work when computing vtable vcall visibility (authored by tejohnson).
Avoid redundant work when computing vtable vcall visibility
Nov 24 2020, 12:06 PM
tejohnson closed D91676: Avoid redundant work when computing vtable vcall visibility.
Nov 24 2020, 12:06 PM · Restricted Project
tejohnson added inline comments to D91676: Avoid redundant work when computing vtable vcall visibility.
Nov 24 2020, 10:16 AM · Restricted Project
tejohnson updated the diff for D91676: Avoid redundant work when computing vtable vcall visibility.

Improve comment

Nov 24 2020, 10:14 AM · Restricted Project
tejohnson added inline comments to D91676: Avoid redundant work when computing vtable vcall visibility.
Nov 24 2020, 9:38 AM · Restricted Project
tejohnson committed rG6e4c1cf29388: [ThinLTO/WPD] Enable -wholeprogramdevirt-skip in ThinLTO backends (authored by tejohnson).
[ThinLTO/WPD] Enable -wholeprogramdevirt-skip in ThinLTO backends
Nov 24 2020, 9:35 AM
tejohnson closed D91812: [ThinLTO/WPD] Enable -wholeprogramdevirt-skip in ThinLTO backends.
Nov 24 2020, 9:35 AM · Restricted Project, Restricted Project
tejohnson added a comment to D91676: Avoid redundant work when computing vtable vcall visibility.

Adding another reviewer - @wmi can you take a look? This is a straightforward compile time fix.

Nov 24 2020, 8:26 AM · Restricted Project
tejohnson added a reviewer for D91676: Avoid redundant work when computing vtable vcall visibility: wmi.
Nov 24 2020, 8:25 AM · Restricted Project

Nov 23 2020

tejohnson added a comment to D91676: Avoid redundant work when computing vtable vcall visibility.

ping

Nov 23 2020, 9:35 AM · Restricted Project

Nov 20 2020

tejohnson added a comment to D91567: [llvm][inliner] Reuse the inliner pass to implement 'always inliner'.

From a ThinLTO perspective, no specific concerns as the buildModuleSimplificationPipeline is invoked in both the pre and post LTO link pipelines, so they both get an equivalent change. But there is an issue for regular LTO, noted below.

Nov 20 2020, 9:20 AM · Restricted Project, Restricted Project

Nov 19 2020

tejohnson accepted D91410: [llvm][clang][mlir] Add checks for the return values from Target::createXXX to prevent protential null deref.

LGTM

Nov 19 2020, 8:16 PM · Restricted Project, Restricted Project, Restricted Project
tejohnson requested review of D91812: [ThinLTO/WPD] Enable -wholeprogramdevirt-skip in ThinLTO backends.
Nov 19 2020, 11:47 AM · Restricted Project, Restricted Project
tejohnson accepted D91585: [NPM] Move more O0 pass building into PassBuilder.

lgtm

Nov 19 2020, 10:37 AM · Restricted Project, Restricted Project
tejohnson added a comment to D91583: [LTO] Prevent devirtualization for symbols exported to dynamic linker.
In D91583#2406005, @pcc wrote:
In D91583#2404519, @pcc wrote:

I've said before that I think that --lto-whole-program-visibility should relax visibility of vtable symbols etc to hidden. That way, --export-dynamic wouldn't actually allow you to make this kind of mistake.

That would presumably result in an error in some of the problematic cases, whereas here we want to simply suppress --lto-whole-program-visibility to avoid any issues automatically.

But isn't it the case that you don't even need for the vtable symbol itself to be exported in order to derive from the class and override its virtual methods?

That's true, but it's the same situation that we have now with --lto-whole-program-visibility and not passing --export-dynamic.

Nov 19 2020, 10:27 AM · Restricted Project
tejohnson committed rGa75b2e87e6cb: [MemProf] Add interface to dump profile (authored by tejohnson).
[MemProf] Add interface to dump profile
Nov 19 2020, 10:22 AM
tejohnson closed D91768: [MemProf] Add interface to dump profile.
Nov 19 2020, 10:22 AM · Restricted Project
tejohnson added inline comments to D91585: [NPM] Move more O0 pass building into PassBuilder.
Nov 19 2020, 10:13 AM · Restricted Project, Restricted Project
tejohnson committed rG8f778b283de5: [sanitizer_common] Add facility to get the full report path (authored by tejohnson).
[sanitizer_common] Add facility to get the full report path
Nov 19 2020, 9:19 AM
tejohnson closed D91765: [sanitizer_common] Add facility to get the full report path.
Nov 19 2020, 9:19 AM · Restricted Project
tejohnson updated the diff for D91765: [sanitizer_common] Add facility to get the full report path.

Beef up test as suggested to check the full path via an assert

Nov 19 2020, 9:19 AM · Restricted Project
tejohnson added inline comments to D91585: [NPM] Move more O0 pass building into PassBuilder.
Nov 19 2020, 9:18 AM · Restricted Project, Restricted Project
tejohnson added a comment to D91474: [buildbot] Fix worker for ThinLTO whole program devirtualization.

Ping - @gkistanova can you take a look and also let me know if there is a way to test the LTO builder change?

Nov 19 2020, 8:55 AM

Nov 18 2020

tejohnson added inline comments to D91765: [sanitizer_common] Add facility to get the full report path.
Nov 18 2020, 11:44 PM · Restricted Project
tejohnson updated the diff for D91765: [sanitizer_common] Add facility to get the full report path.

Fix test to avoid using /tmp

Nov 18 2020, 11:24 PM · Restricted Project
tejohnson added inline comments to D91765: [sanitizer_common] Add facility to get the full report path.
Nov 18 2020, 11:24 PM · Restricted Project
tejohnson requested review of D91768: [MemProf] Add interface to dump profile.
Nov 18 2020, 11:15 PM · Restricted Project
tejohnson added a comment to D91765: [sanitizer_common] Add facility to get the full report path.

It mentions the test, but it's not here

Nov 18 2020, 10:11 PM · Restricted Project
tejohnson updated the diff for D91765: [sanitizer_common] Add facility to get the full report path.

Add the test

Nov 18 2020, 10:10 PM · Restricted Project
tejohnson added a comment to D91583: [LTO] Prevent devirtualization for symbols exported to dynamic linker.
In D91583#2404519, @pcc wrote:

I've said before that I think that --lto-whole-program-visibility should relax visibility of vtable symbols etc to hidden. That way, --export-dynamic wouldn't actually allow you to make this kind of mistake.

Nov 18 2020, 10:09 PM · Restricted Project
tejohnson requested review of D91765: [sanitizer_common] Add facility to get the full report path.
Nov 18 2020, 9:58 PM · Restricted Project
tejohnson added a comment to D91583: [LTO] Prevent devirtualization for symbols exported to dynamic linker.

Thanks for the clarification.

Hi, I am still learning the feature and I've just played a bit with the test. I've a couple of questions:

  • The --export-dynamic usage in the test seems a bit confusing. Are the VisibleToRegularObj bits of these _ZTV* symbols the important matter and --export-dynamic is just an approach to make them true?

Correct. It was just a shorthand way of preventing these symbols from being eliminated.

  • If yes, I guess all these --export-dynamic can be replaced with -u _ZTV1B -u _ZTV1C -u _ZTV1D in all the RUN lines. --export-dynamic is preferred just due to its brevity. If that is the case, I think this deserves a comment considering its subtle interaction with noexportdynamic.

Right, that's why for one of the tests where I'm using the new noexportdynamic value I switched to the -u sequence instead. I can add a comment.

  • When is devirtualization invalid? For example, if _ZTV1D is exported to the dynamic symbol table and a shared object inherits from class D and overrides the method?

Correct. If it is both exported and then overridden.

If my last point is correct, I'd agree that we need something like a tri-state option. (I hope we can remove --lto-whole-program-visibility if possible)
on/noexportdynamic the value names do not capture the actual meaning. The on actually means: devirtualization is safe as long as the used _ZTV* symbols are not exported.

Suggestion on the name? It's basically an assertion, i.e. that there is LTO whole program visibility (no, when --export-dynamic not specified, and yes). 'on' means the user is asserting that the _ZTV* are not going to be exported and overridden.

I have a further question: is it realistic to add another bit along with VisibleToRegularObj to convey the information whether a symbol is includeInDynsym()?
This way virtual functions related to individual _ZTV* can be safely devirtualized, no matter how users specify --export-dynamic-symbol,--export-dynamic,--dynamic-list or add link-time shared objects to alter the includeInDynsym() state of _ZTV* symbols.

Nov 18 2020, 6:14 PM · Restricted Project
tejohnson added inline comments to D91687: [compiler-rt santizer] Use clock_gettime instead of timespec_get.
Nov 18 2020, 3:35 PM · Restricted Project
tejohnson added a comment to D85808: [Remarks][2/2] Expand remarks hotness threshold option support in more tools.

Thanks for adding the Driver test. I was thinking of something to test the CompilerInvocation changes, similar to your test using opt, that ensures the option has the desired behavior when invoked via clang. Looks like there is an existing test clang/test/Frontend/optimization-remark-with-hotness.c that perhaps could be extended or leveraged?

Nov 18 2020, 11:59 AM · Restricted Project, Restricted Project
tejohnson added a comment to D91410: [llvm][clang][mlir] Add checks for the return values from Target::createXXX to prevent protential null deref.

... the new checking is a mix of assert and fatal errors. Is that intended?

No. The added checks are based on the checks on other calls to the Target::createXXX functions in this file or other related files. If there are any fatal errors nearby (e.g. llvm/lib/LTO/ThinLTOCodeGenerator.cpp:581 vs 569), the check will be a fatal error; and if there are any assertions (e.g. llvm/lib/CodeGen/LLVMTargetMachine.cpp:43,45,52 vs 60) or the calls are never checked (e.g. llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:300), the added check will be an assertion.

If these are not likely due to user input issues, then perhaps they should all be assert so that they are compiled out in release compilers?

Since all these problems are reported by my static analyzer, I do not really know whether these checked pointers will actually be null when the code is executed. And I did not try to dynamically execute the program to check the problems either. But chances are that if the creator callbacks are not properly set or the called creator functions returns nullptr, the problem will happen. In my opinion, these problems may only happen during development. Therefore, I believe asserts can be sufficient to diagnose the problems.

If you think it would be better to use assertions instead of fatal errors, I will make an update on all llvm/lib/xxx files (or maybe all files). But before that, I'd prefer waiting for the replies from other reviewers on the remaining parts of the patch and making an update for all the suggestions.

Nov 18 2020, 7:45 AM · Restricted Project, Restricted Project, Restricted Project

Nov 17 2020

tejohnson requested review of D91676: Avoid redundant work when computing vtable vcall visibility.
Nov 17 2020, 8:05 PM · Restricted Project
tejohnson added a comment to D91583: [LTO] Prevent devirtualization for symbols exported to dynamic linker.

Hi, I am still learning the feature and I've just played a bit with the test. I've a couple of questions:

  • The --export-dynamic usage in the test seems a bit confusing. Are the VisibleToRegularObj bits of these _ZTV* symbols the important matter and --export-dynamic is just an approach to make them true?
Nov 17 2020, 3:37 PM · Restricted Project
tejohnson added a comment to D91585: [NPM] Move more O0 pass building into PassBuilder.

Thanks, generally this seems to be good cleanup. Question on one of the changes below though.

Nov 17 2020, 12:41 PM · Restricted Project, Restricted Project
tejohnson added a comment to D85808: [Remarks][2/2] Expand remarks hotness threshold option support in more tools.

Largely LGTM, except that the clang changes are missing a corresponding test.

Nov 17 2020, 9:10 AM · Restricted Project, Restricted Project