Projects

User does not belong to any projects.

User Details

User Since
Jan 18 2017, 9:47 AM (321 w, 5 d)

Nov 22 2022

The change itself seems fine, but the CI failure seems related to this change.

Nov 22 2022, 1:50 PM · Restricted Project, Restricted Project

Fix for test failure on *nix.

Nov 22 2022, 1:43 PM · Restricted Project, Restricted Project

Nov 21 2022

Nov 21 2022, 5:04 PM · Restricted Project, Restricted Project

Nov 16 2022

bd1976llvm updated the diff for D88124: [windows-itanium] [WIP] Windows Itanium Build Recipe.

Set DLL storage class for guard variables correctly to avoid a link error when code using a guard variable is inlined into a module that imports the code.

Nov 16 2022, 3:19 PM · Restricted Project

Oct 18 2022

Oct 18 2022, 4:45 AM · Restricted Project, Restricted Project
Oct 18 2022, 4:45 AM · Restricted Project, Restricted Project

Oct 17 2022

Oct 17 2022, 4:53 PM · Restricted Project, Restricted Project

Oct 13 2022

Abandoned in favour of: https://reviews.llvm.org/D135897.

Oct 13 2022, 11:02 AM · Restricted Project
Oct 13 2022, 11:01 AM · Restricted Project, Restricted Project
bd1976llvm added inline comments to D135737: [LLD][ELF] restore behaviour of __real_foo being a weak reference to a lazy foo.
Oct 13 2022, 10:51 AM · Restricted Project

(--wrap is so complex that I can't come up with a list of rules to follow. I can only make sense by inspecting test output difference.)

For wrap-extract-real.s, I don't think the new behavior improves semantics.

```ld.lld %t/a.o --wrap foo  # error: undefined symbol: __real_foo
ld.lld %t/a.o --start-lib %t/b.o --end-lib --wrap foo  # ok, even if %t/b.o is not extracted. This is weird```

Generally, if an archive member is not extracted, we expect that there is no symbol resolution difference.

Oct 13 2022, 10:45 AM · Restricted Project

Oct 12 2022

Improve testing and add tests to show that symbol foo is considered referenced by LLD even if all references are redirected by --wrap foo. This property means that a __wrap_foo function that contains calls to __real_foo will not be called if foo is not included in the link.

Oct 12 2022, 9:16 AM · Restricted Project
Oct 12 2022, 9:08 AM · Restricted Project

Oct 11 2022

bd1976llvm retitled D135737: [LLD][ELF] restore behaviour of __real_foo being a weak reference to a lazy foo from [LLD][ELF] restore behaviour of __real_foo being a weak_reference_to_a_lazy_foo to [LLD][ELF] restore behaviour of __real_foo being a weak reference to a lazy foo.
Oct 11 2022, 6:53 PM · Restricted Project
Oct 11 2022, 6:40 PM · Restricted Project
Oct 11 2022, 6:38 PM · Restricted Project

Sep 29 2022

Sep 29 2022, 4:28 PM · Restricted Project, Restricted Project
bd1976llvm updated the diff for D134784: [IR] Don't allow DLL storage-class and local linkage.

Sep 29 2022, 4:16 PM · Restricted Project, Restricted Project
bd1976llvm added a comment to D134784: [IR] Don't allow DLL storage-class and local linkage.

For Clang, it seems that MSVC allows dllspec on a local linkage variable, so that's why we have to add it to CodeGen instead of rejecting it in Sema.

Can you give an example? MSVC certainly rejects simple cases e.g. https://godbolt.org/z/bG3M7oj5W. I am worried that I have missed something...

__declspec(dllexport) const int foo = 10; foo with internal linkage is accepted.

Sep 29 2022, 4:15 PM · Restricted Project, Restricted Project

Sep 28 2022

bd1976llvm added a comment to D134784: [IR] Don't allow DLL storage-class and local linkage.

For Clang, it seems that MSVC allows dllspec on a local linkage variable, so that's why we have to add it to CodeGen instead of rejecting it in Sema.

Sep 28 2022, 4:02 PM · Restricted Project, Restricted Project
bd1976llvm updated the diff for D134784: [IR] Don't allow DLL storage-class and local linkage.
Sep 28 2022, 6:57 AM · Restricted Project, Restricted Project
bd1976llvm added a comment to D134784: [IR] Don't allow DLL storage-class and local linkage.

Thanks for the review. I addressed your comments and also consolidated the tests into a single test using split-file.

Sep 28 2022, 6:56 AM · Restricted Project, Restricted Project

Sep 27 2022

Whilst reviewing the impact of these dllimport/export restrictions I noticed that LLVM IR does not reject dllimport/export on local linkage globals. I have put up a PR for adding that restriction: D134784 (https://reviews.llvm.org/D134784).

Sep 27 2022, 8:12 PM · Restricted Project, Restricted Project
bd1976llvm requested review of D134784: [IR] Don't allow DLL storage-class and local linkage.
Sep 27 2022, 8:11 PM · Restricted Project, Restricted Project

Sep 21 2022

bd1976llvm added a comment to D134015: [Utils] Refactor update_cc_test_checks.py to use shutil.

Thanks. LGTM.

Sep 21 2022, 11:38 PM · Restricted Project, Restricted Project

Sep 20 2022

bd1976llvm added a comment to D134015: [Utils] Refactor update_cc_test_checks.py to use shutil.

I reverted this change as it breaks the tests on windows. It seems that distutils.spawn.find_executable is not exactly equivalent to shutil.which on Windows as the former adds ".exe" and the latter adds the extensions from PATHEXT. Furthermore we are invoking these functions on a full path e.g. "C:\p\15\vs2019\bin\clang" and looking at the docs I'm not sure what the behaviour of either function should be in that case.

Sep 20 2022, 8:54 AM · Restricted Project, Restricted Project

Sep 12 2022

From a glance, -fvisibility-global-new-delete-hidden should make the visibility implicit (so won't trigger this error) but don't seem to do so? I'll investigate later.

-fvisibility-global-new-delete-hidden is implemented by adding VisibilityAttr to declarations.
I think -fvisibility-global-new-delete-hidden triggering the error is fine. The alternative, adding a rule that __declspec(dllexport) void operator delete does not get hidden visibility, seems ad-hoc to me.

I'm not sure why this visibility option (-fvisibility-global-new-delete-hidden) is implemented differently to the others (e.g. -fvisibility=hidden)? __declspec(dllexport) void operator delete does not get hidden visibility, might be difficult to implement; but OTOH, the dllstorageclass overrides the visibility set by a visibility option for the other visibility options (e.g. -fvisibility=hidden) and it would be nice to have consistent behaviour for all the visibility options. Would be great to get other peoples opinions on whether an error here would be a problem?

First, does COFF use -fvisibility-global-new-delete-hidden? The impl of -fvisibility-global-new-delete-hidden is very different from -fvisibility. -fvisibility changed visibility is considered !isVisibilityExplicit while -fvisibility-global-new-delete-hidden's is isVisibilityExplicit.

dllspec and visibility are extensions for different object file formats and here we are mixing them together.
Personally I don't favor defining too many "let A override B" designs. Adding if (GV->hasDLLExportStorageClass() || GV->hasDLLImportStorageClass()) is mostly only due to compatibility consideration...

Sep 12 2022, 3:24 PM · Restricted Project, Restricted Project

Sep 7 2022

From a glance, -fvisibility-global-new-delete-hidden should make the visibility implicit (so won't trigger this error) but don't seem to do so? I'll investigate later.

-fvisibility-global-new-delete-hidden is implemented by adding VisibilityAttr to declarations.
I think -fvisibility-global-new-delete-hidden triggering the error is fine. The alternative, adding a rule that __declspec(dllexport) void operator delete does not get hidden visibility, seems ad-hoc to me.

Sep 7 2022, 5:40 PM · Restricted Project, Restricted Project

This approach doesn't account for the implementation of -fvisibility-global-new-delete-hidden. The following case now fails after this change:

Sep 7 2022, 11:57 AM · Restricted Project, Restricted Project
bd1976llvm added a comment to D133267: [Verifier] Reject dllexport with non-default visibility.

Hi! Sorry for not raising this issue prior to commit. The dllimport restriction seems correct to us. However, in PlayStation we are using dllexportclass=dllexport visibility=protected GlobalValues in LLVM IR. The protected visibility indicates that the symbol cannot be preempted and could be exported and dllexport indicates that it should be exported. This allows for implementing export control from C/C++ source code. This change has made this usage illegal. It doesn't makes sense semantically to restrict dllexport to visibility=default in LLVM IR, the restriction should be to visibility != hidden as hidden symbols cannot be exported by ELF linkers. Thanks.

Oh, didn't know. Pushed 97d00b72a2b0a7aca631e1402a647f32c4e8bafb to allow dllexport protected.
Does PlayStation need to relax the clang check D133266, too? (I have noticed that this combo makes sense but rejected it for simplicity.)

Sep 7 2022, 11:52 AM · Restricted Project, Restricted Project

Sep 6 2022

bd1976llvm added a comment to D133267: [Verifier] Reject dllexport with non-default visibility.

Hi! Sorry for not raising this issue prior to commit. The dllimport restriction seems correct to us. However, in PlayStation we are using dllexportclass=dllexport visibility=protected GlobalValues in LLVM IR. The protected visibility indicates that the symbol cannot be preempted and could be exported and dllexport indicates that it should be exported. This allows for implementing export control from C/C++ source code. This change has made this usage illegal. It doesn't makes sense semantically to restrict dllexport to visibility=default in LLVM IR, the restriction should be to visibility != hidden as hidden symbols cannot be exported by ELF linkers. Thanks.

Sep 6 2022, 6:33 AM · Restricted Project, Restricted Project

Jul 1 2022

Jul 1 2022, 6:01 AM · Restricted Project, Restricted Project

Jun 30 2022

at link time is incorrect for distributed ThinLTO which happens at clang -fthinlto-index=... time. You may just say "ThinLTO backend compilation phase"

Jun 30 2022, 3:00 PM · Restricted Project, Restricted Project
Jun 30 2022, 2:59 PM · Restricted Project, Restricted Project
bd1976llvm retitled D128452: [LLVM][LTO][LLD] Enable Profile Guided Layout (--call-graph-profile-sort) for FullLTO from [LLVM][LTO][LLD] Enable Profile Guided Layout (--call-graph-profile-sort) for FullLTO (PR #56185) to [LLVM][LTO][LLD] Enable Profile Guided Layout (--call-graph-profile-sort) for FullLTO.
Jun 30 2022, 2:58 PM · Restricted Project, Restricted Project

Jun 30 2022, 11:16 AM · Restricted Project, Restricted Project
Jun 30 2022, 10:56 AM · Restricted Project, Restricted Project
bd1976llvm retitled D128452: [LLVM][LTO][LLD] Enable Profile Guided Layout (--call-graph-profile-sort) for FullLTO from [PGL][LTO] Make PGL work with FLTO (PR #56185) to [LLVM][LTO][LLD] Enable Profile Guided Layout (--call-graph-profile-sort) for FullLTO (PR #56185).
Jun 30 2022, 6:41 AM · Restricted Project, Restricted Project

Jun 29 2022

Added some reviewer from D112160 where PGL has recently been made to work for macho LLD. It seems likely (to me!) that macho is also affected by this issue.

Jun 29 2022, 1:30 AM · Restricted Project, Restricted Project
Jun 29 2022, 1:29 AM · Restricted Project, Restricted Project

Jun 28 2022

(Not familiar with CGProfilePass and its requirements)

Jun 28 2022, 6:56 PM · Restricted Project, Restricted Project
Jun 28 2022, 6:33 PM · Restricted Project, Restricted Project

Jun 23 2022

Jun 23 2022, 9:39 AM · Restricted Project, Restricted Project
bd1976llvm updated the diff for D128195: [LLD][ELF] Add FORCE_LLD_DIAGNOSTICS_CRASH to force LLD to crash.
• Removed the LLD_IN_TEST un-setting in the lit test as we have moved the code earlier now.
• Removed checking for the period/full-stop in the lit test to allow customizers to extend the sentence without adjusting this test.
Jun 23 2022, 8:25 AM · Restricted Project, Restricted Project
bd1976llvm added inline comments to D128195: [LLD][ELF] Add FORCE_LLD_DIAGNOSTICS_CRASH to force LLD to crash.
Jun 23 2022, 6:41 AM · Restricted Project, Restricted Project
bd1976llvm updated the diff for D128195: [LLD][ELF] Add FORCE_LLD_DIAGNOSTICS_CRASH to force LLD to crash.

Ensure added lit test passes for LLVM_ENABLE_BACKTRACES=off builds.
Added differential URL to release note.
Used regex so that test doesn't care about contents of the bug report URL.

Jun 23 2022, 6:40 AM · Restricted Project, Restricted Project

Jun 22 2022

bd1976llvm added inline comments to D128195: [LLD][ELF] Add FORCE_LLD_DIAGNOSTICS_CRASH to force LLD to crash.
Jun 22 2022, 4:22 PM · Restricted Project, Restricted Project
bd1976llvm updated the diff for D128195: [LLD][ELF] Add FORCE_LLD_DIAGNOSTICS_CRASH to force LLD to crash.

Move to lld.cpp

Jun 22 2022, 4:18 PM · Restricted Project, Restricted Project

Jun 21 2022

bd1976llvm updated the diff for D128195: [LLD][ELF] Add FORCE_LLD_DIAGNOSTICS_CRASH to force LLD to crash.

Remove accidentally committed comments.

Jun 21 2022, 5:53 AM · Restricted Project, Restricted Project
bd1976llvm updated the diff for D128195: [LLD][ELF] Add FORCE_LLD_DIAGNOSTICS_CRASH to force LLD to crash.

Fix typo.

Jun 21 2022, 5:49 AM · Restricted Project, Restricted Project
bd1976llvm updated the diff for D128195: [LLD][ELF] Add FORCE_LLD_DIAGNOSTICS_CRASH to force LLD to crash.

Jun 21 2022, 5:47 AM · Restricted Project, Restricted Project

Jun 20 2022

bd1976llvm updated the diff for D128195: [LLD][ELF] Add FORCE_LLD_DIAGNOSTICS_CRASH to force LLD to crash.

Jun 20 2022, 1:31 PM · Restricted Project, Restricted Project

Entry in ReleaseNotes or man page?

Jun 20 2022, 1:31 PM · Restricted Project, Restricted Project

Alternatively would a --crash option be easier to manage? I don't think we can replicate the reproducer that the clang driver uses with this option.

Jun 20 2022, 8:53 AM · Restricted Project, Restricted Project
bd1976llvm updated the diff for D128195: [LLD][ELF] Add FORCE_LLD_DIAGNOSTICS_CRASH to force LLD to crash.

Added useful reminder message as suggested by Peter.

Jun 20 2022, 8:52 AM · Restricted Project, Restricted Project
Jun 20 2022, 5:58 AM · Restricted Project, Restricted Project

I have made a commit with the current improvements.

Jun 20 2022, 5:13 AM · Restricted Project, Restricted Project

Jun 17 2022

how's this compare to the linux behavior? be nice to have a similar format

Jun 17 2022, 9:45 AM · Restricted Project, Restricted Project

I have asked around and no one made use of those numbers so I have removed them in the latest diff. However, people would have found the exception code useful so I have added that. Output now looks like:

Jun 17 2022, 9:43 AM · Restricted Project, Restricted Project

Jun 15 2022

Jun 15 2022, 4:32 PM · Restricted Project, Restricted Project
Jun 15 2022, 4:19 PM · Restricted Project, Restricted Project

May 27 2022

bd1976llvm retitled D125785: [llvm-ar][test] add special case of replace converting a regular to a thin archive from [llvm-ar][test] test special case of replace converting a regular to a thin archive to [llvm-ar][test] add special case of replace converting a regular to a thin archive.
May 27 2022, 3:04 AM · Restricted Project, Restricted Project
bd1976llvm retitled D125785: [llvm-ar][test] add special case of replace converting a regular to a thin archive from [llvm-ar][test] Add regression test for special case of replace converting a regular to a full archive to [llvm-ar][test] test special case of replace converting a regular to a thin archive.
May 27 2022, 3:04 AM · Restricted Project, Restricted Project

Sorry for the delay. I wanted to wait until https://github.com/llvm/llvm-project/issues/55527 was fully understood in case we needed to adjust the test or add extra cases.

May 27 2022, 3:00 AM · Restricted Project, Restricted Project
May 27 2022, 2:58 AM · Restricted Project, Restricted Project
bd1976llvm retitled D125785: [llvm-ar][test] add special case of replace converting a regular to a thin archive from [llvm-ar][test] Add regression test for special case of replace converting a regular to a full archive (issue 55527) to [llvm-ar][test] Add regression test for special case of replace converting a regular to a full archive.
May 27 2022, 2:53 AM · Restricted Project, Restricted Project

May 18 2022

May 18 2022, 4:03 AM · Restricted Project, Restricted Project

May 17 2022

update diff to a working regression test!

May 17 2022, 6:52 AM · Restricted Project, Restricted Project
May 17 2022, 6:51 AM · Restricted Project, Restricted Project
bd1976llvm added a comment to D123949: [AIX] support write operation of big archive..

@DiggerLin there are a number of tests e.g: llvm\test\tools\llvm-ar\regular-to-thin-archive.test that are still marked "XFAIL: system-aix" that probably need looking at. It might be good to write down what testing strategy you want for AIX. Are you happy to rely on --format=gnu test coverage for most of the tests and have a few specifically --format=aix tests?

May 17 2022, 3:09 AM · Restricted Project, Restricted Project

May 16 2022

@MaskRay - thanks for the review comments, I will keep your advise w.r.t. urls in commit comments in mind.

May 16 2022, 4:52 PM · Restricted Project, Restricted Project
bd1976llvm added a comment to D123949: [AIX] support write operation of big archive..

Hi @DiggerLin. Currently the --help text states --format=aix but llvm-ar accepts --format=bigarchive. Could you fix that please.

May 16 2022, 3:48 PM · Restricted Project, Restricted Project

@bd1976llvm do you have an ETA to address the AIX failure Jake mentions above?

May 16 2022, 2:53 PM · Restricted Project, Restricted Project

May 13 2022

May 13 2022, 9:43 AM · Restricted Project, Restricted Project

May 13 2022, 9:43 AM · Restricted Project, Restricted Project

May 12 2022

May 12 2022, 8:44 AM · Restricted Project, Restricted Project

Now using split-file and cd. Removed an invalid test-case. Addressed review comments.

May 12 2022, 8:43 AM · Restricted Project, Restricted Project

May 12 2022, 7:30 AM · Restricted Project, Restricted Project
May 12 2022, 7:29 AM · Restricted Project, Restricted Project

May 11 2022

May 11 2022, 7:07 PM · Restricted Project, Restricted Project
May 11 2022, 7:05 PM · Restricted Project, Restricted Project

Feb 15 2022

bd1976llvm added a comment to D119074: [ELF] Parse archives as --start-lib object files.

I am slightly concerned about penalising links with low utilisation archives. However, I don't have any specific data on low utilisation archives or any example of them. We did do some testing of a similar feature (in our proprietary linker) on game code, and low utilisation archives did not cause a problem for any of the games tested. Users should be able to create high utilisation versions of problematic archives in any case.

Feb 15 2022, 9:22 AM · Restricted Project
bd1976llvm added inline comments to D119074: [ELF] Parse archives as --start-lib object files.
Feb 15 2022, 1:15 AM · Restricted Project

Feb 2 2022

bd1976llvm added a comment to D118577: [ELF] Deduplicate names of local symbols only with -O2.

This .strtab deduplication makes parallel .symtab write difficult. I suspect we may need to disable it entirely in the future.

Feb 2 2022, 1:36 AM · Restricted Project

Jan 24 2022

Currently a deplib name (\${name}) indicates either the literal \${name} or the -l style lib\${name}.{a,so}.
We have a conflict if \${name} is foo and there is a directory named foo.

What if we do

```if (name.endswith(".a") || name.endswith(".so"))
findFromSearchPaths
else
searchLibraryBaseName```

?

That should avoid the conflict, too. A directory is unlikely named .so or .a.

I don't think introducing more magical rules about naming patterns ever makes things easier.

It is surprising at first blush that deplibs searches for "foo" and not just what "-lfoo" would do, but it makes sense if you want to allow saying "libfoo.a" in deplibs to avoid matching "libfoo.so" in arcane cases.
So I think just the fix here to make all cases of not-existing-files candidates on search paths be ignored robustly is sufficient. The extra set of candidates that deplibs cases will attempt is tolerable and IMHO it's far preferable to having any more magic in the semantics of the behavior than there already is

It's not adding more magical rules. The chained findFromSearchPaths and searchLibraryBaseName is replaced with testing just one form.
We can then get rid of llvm::function_ref<bool(StringRef)>.

Jan 24 2022, 3:59 PM · Restricted Project

Jan 19 2022

bd1976llvm added a comment to D117284: [ELF] Allow non-bitcode archive with an empty index.

No objections. Reading your blog post and my copy of Linkers and Loaders it does seem that early linkers would handle archives with indexes, it just seems like indexes were ubiquitous when GNU ld were written. So it does seem like rejecting archives without an index is a property of the implentation of GNU ld rather than something that should be relied upon.

Jan 19 2022, 7:51 AM · Restricted Project

Jan 14 2022

bd1976llvm added a comment to D117284: [ELF] Allow non-bitcode archive with an empty index.

Another advantage is that this behaviour allows users to "repair" archives with incomplete symbol tables by stripping the symbol table (maybe someone has added both ET_RELs and bitcode objects to the same archive with an ar which ignored the bitcocde symbols).

Isn't it already trivial to "repair" such libraries by running ranlib on them?

Jan 14 2022, 5:52 AM · Restricted Project
bd1976llvm added a comment to D117284: [ELF] Allow non-bitcode archive with an empty index.

Another advantage is that this behaviour allows users to "repair" archives with incomplete symbol tables by stripping the symbol table (maybe someone has added both ET_RELs and bitcode objects to the same archive with an ar which ignored the bitcocde symbols).

Jan 14 2022, 3:24 AM · Restricted Project

Dec 9 2021

Sorry I missed this review - looks great! Did you try getting performance numbers using the tar packages that Rafael gathered up?

Dec 9 2021, 1:54 AM · Restricted Project

Oct 11 2021

Sorry for the slow response. I was out of town for one week.

Oct 11 2021, 5:18 AM · Restricted Project

Do not test reloc offset as its value isn't important.

Oct 11 2021, 5:08 AM · Restricted Project

Sep 30 2021

Additionally augmented weak-undef.s and weak-undef-rw.s with a --hex-dump test.

Sep 30 2021, 6:25 PM · Restricted Project

Sep 29 2021

Sep 29 2021, 7:14 AM · Restricted Project
Sep 29 2021, 7:13 AM · Restricted Project

Sep 23 2021

bd1976llvm updated the summary of D109804: [LLD] [ELF] Add emission of an optional linkmap section .
Sep 23 2021, 5:42 PM · Restricted Project

Sep 20 2021

Thanks for working on this. In the past I have resorted to hacking in my own ad-hoc versions of such features to solve particularly horrible dependency cases. As mentioned above there's a case for more powerful dependency tracking e.g. https://reviews.llvm.org/D69607. The more of these debugging features available the better IMO :)

Sep 20 2021, 6:17 PM · Restricted Project

Sep 16 2021

bd1976llvm updated the summary of D109804: [LLD] [ELF] Add emission of an optional linkmap section .
Sep 16 2021, 3:44 PM · Restricted Project
bd1976llvm updated the summary of D109804: [LLD] [ELF] Add emission of an optional linkmap section .
Sep 16 2021, 9:25 AM · Restricted Project