Page MenuHomePhabricator

glandium (Mike Hommey)
User

Projects

User does not belong to any projects.

User Details

User Since
Nov 7 2017, 12:52 AM (243 w, 2 d)

Recent Activity

Tue, Jun 21

glandium added a comment to D107799: [CMake] Enable LLVM_ENABLE_PER_TARGET_RUNTIME_DIR by default on Linux.

https://github.com/llvm/llvm-project/commit/716e27bc9ad4699bbc82318834e938bcc9682cf8 turned this on for arm linux systems. Having this on by default in some configs on linux and off on others seems like it'd be very confusing. Let's please either have it on or off uniformly per OS?

Tue, Jun 21, 1:01 PM · Restricted Project, Restricted Project, Restricted Project

Thu, Jun 16

glandium added inline comments to D127828: [libFuzzer] Use the compiler to link the relocatable object.
Thu, Jun 16, 1:54 AM · Restricted Project, Restricted Project
glandium added inline comments to D127828: [libFuzzer] Use the compiler to link the relocatable object.
Thu, Jun 16, 1:53 AM · Restricted Project, Restricted Project

Wed, Jun 15

glandium added a comment to D107799: [CMake] Enable LLVM_ENABLE_PER_TARGET_RUNTIME_DIR by default on Linux.

I don't know how this works for you. CMake gets a list of implicit link dirs from parsing the ld command that clang -v outputs when linking. When $clang/lib/x86_64-unknown-linux-gnu exists, that's part of that output. Then, CMake determines the arch it later uses when finding libraries by matching architectures in each of the implicit link dirs, and $clang/lib/x86_64-unknown-linux-gnu is the first match. From there [it uses x86_64-unknown-linux-gnu](https://github.com/Kitware/CMake/blob/master/Source/cmSearchPath.cxx#L180), and of course it doesn't find libraries in /usr/lib/x86_64-linux-gnu because it never looks there in find_library. That can even affect pkg-config, because it uses that same derived architecture to find pkgconfig directories. When $clang/lib/x86_64-unknown-linux-gnu doesn't exist, /lib/x86_64-linux-gnu is the first match, and it uses x86_64-linux-gnu as arch, and that works.

Wed, Jun 15, 9:07 PM · Restricted Project, Restricted Project, Restricted Project
glandium added inline comments to D127828: [libFuzzer] Use the compiler to link the relocatable object.
Wed, Jun 15, 8:49 PM · Restricted Project, Restricted Project

Fri, Jun 10

glandium added a comment to D107799: [CMake] Enable LLVM_ENABLE_PER_TARGET_RUNTIME_DIR by default on Linux.

Do you mean it found zlib?

Fri, Jun 10, 12:19 AM · Restricted Project, Restricted Project, Restricted Project

Thu, Jun 9

glandium added a comment to D126898: [COFF] Check table ptr more thoroughly and ignore empty sections.

Passing -Wl,/fixed as a linker flag when building the NSIS stubs fixes the issue, FWIW.

Thu, Jun 9, 9:59 PM · Restricted Project, Restricted Project
glandium added a comment to D126898: [COFF] Check table ptr more thoroughly and ignore empty sections.

So, looking a little bit deeper, the resource editor in NSIS doesn't handle relocations, and the .reloc section is right after the .rsrc section that the resource editor will grow... So they actually have a build rule to pass /FIXED to link.exe when building their stubs with MSVC. I suppose mingw-binutils defaults to non-relocatable .exes (because the problem doesn't appear when building with mingw-gcc/mingw-binutils), and we only end up with relocatable .exes because we're building with mingw-clang/lld and lld defaults to relocatable .exes.
I'd call this a build system bug.

Thu, Jun 9, 9:04 PM · Restricted Project, Restricted Project
glandium added a comment to D126898: [COFF] Check table ptr more thoroughly and ignore empty sections.

The base relocation table usually resides in the .reloc section, and as the exact size (0xB68) matches, it looks like it just has got the wrong RVA in the header. With the new, more thorough checks, libObject notices that the RVA points into the .ndata section (within 0x40000 - 0x5C000) but there's only raw data covering 0x40000 - 0x40200, and 0x43000 is outside of that. Previously we'd just set up a pointer into bogus/out of range data in these cases, now we error out.

Thu, Jun 9, 5:19 PM · Restricted Project, Restricted Project
glandium added a comment to D107799: [CMake] Enable LLVM_ENABLE_PER_TARGET_RUNTIME_DIR by default on Linux.

Minimal reproducer:

  • git checkout 311f7839602344ca347816146edb68c0ffaaa060
  • cmake llvm -B obj -G Ninja -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX=/tmp/llvm/stage1 -DLLVM_TARGETS_TO_BUILD=X86 -DLLVM_ENABLE_PROJECTS="clang" -DLLVM_ENABLE_RUNTIMES="compiler-rt;libcxx;libcxxabi"
  • ninja -C obj install
  • cmake llvm -B obj2 -G Ninja -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_PROJECTS="clang" -DCMAKE_C_COMPILER=/tmp/llvm/stage1/bin/clang
Thu, Jun 9, 1:31 AM · Restricted Project, Restricted Project, Restricted Project

Wed, Jun 8

glandium added a comment to D126898: [COFF] Check table ptr more thoroughly and ignore empty sections.

Can you upload that binary somewhere?

Wed, Jun 8, 4:11 AM · Restricted Project, Restricted Project
glandium added a comment to D126898: [COFF] Check table ptr more thoroughly and ignore empty sections.

I don't have time to provide more details right now, but this change broke Firefox mingw builds with: llvm-strip: error: '../../dist/firefox/uninstall/helper.exe': RVA 0x43000 for base reloc table found but data is incomplete

Wed, Jun 8, 2:21 AM · Restricted Project, Restricted Project
glandium added a comment to D107799: [CMake] Enable LLVM_ENABLE_PER_TARGET_RUNTIME_DIR by default on Linux.

But it is irrelevant here

Wed, Jun 8, 12:59 AM · Restricted Project, Restricted Project, Restricted Project
glandium added a comment to D107799: [CMake] Enable LLVM_ENABLE_PER_TARGET_RUNTIME_DIR by default on Linux.

What is your $HOME/Stable/bin/clang? it has to be a clang with the patch applied.

Wed, Jun 8, 12:44 AM · Restricted Project, Restricted Project, Restricted Project

Tue, Jun 7

glandium added a comment to D107799: [CMake] Enable LLVM_ENABLE_PER_TARGET_RUNTIME_DIR by default on Linux.

IOW, nothing changed since https://reviews.llvm.org/D107799#3027607

Tue, Jun 7, 11:35 PM · Restricted Project, Restricted Project, Restricted Project
glandium added a comment to D107799: [CMake] Enable LLVM_ENABLE_PER_TARGET_RUNTIME_DIR by default on Linux.

I suspect even eliminating the machine part of the target, there could still be problems with i386 vs i686.

Tue, Jun 7, 11:24 PM · Restricted Project, Restricted Project, Restricted Project
glandium added a comment to D107799: [CMake] Enable LLVM_ENABLE_PER_TARGET_RUNTIME_DIR by default on Linux.

This is causing problems finding system zlib when compiling clang with clang, because when using the target directories for runtimes, cmake determines the target name to match that, and then proceeds to try to find zlib.h in /usr/include/$target, which doesn't work when $target doesn't match the system multi-arch triple, which it doesn't on Debian systems because the runtime directory and the cmake target is e.g. x86_64-unknown-linux-gnu, and the system multiarch triple is x86_64-linux-gnu...

Tue, Jun 7, 11:23 PM · Restricted Project, Restricted Project, Restricted Project

Apr 12 2022

glandium added a comment to D118214: [ObjCARC] Require the function argument in the clang.arc.attachedcall bundle..
In D118214#3446233, @ab wrote:

D'oh, this fell through the cracks, sorry folks! I committed the fix I had in mind (cfa4fe7c5187).

Apr 12 2022, 11:34 PM · Restricted Project, Restricted Project

Apr 11 2022

glandium added a comment to D118214: [ObjCARC] Require the function argument in the clang.arc.attachedcall bundle..

FWIW, We're seeing Firefox crashes on aarch64 macos that have been bisected to this change as well. Top frames look like:

Apr 11 2022, 11:47 PM · Restricted Project, Restricted Project

Mar 18 2022

Herald added a project to D115319: [lsan] Move out suppression of invalid PCs from StopTheWorld: Restricted Project.

For some reason this change makes Firefox Linux ASAN builds break our CI in a way that doesn't even leave logs (it looks like the workers completely die)

Mar 18 2022, 2:16 PM · Restricted Project, Restricted Project

Mar 10 2022

glandium added a comment to D116709: [CMake][WinMsvc] Fix user passed compiler/linker flags.

It turns out CMAKE_*_FLAGS is only initialized from CMAKE_*_FLAGS_INIT _after_ CMAKE_DETERMINE_COMPILER_ID is done.

Mar 10 2022, 5:34 PM · Restricted Project, Restricted Project
glandium added a comment to D116709: [CMake][WinMsvc] Fix user passed compiler/linker flags.

It still happens when I cut down to CMAKE_TOOLCHAIN_FILE, LLVM_NATIVE_TOOLCHAIN, MSVC_BASE, WINSDK_BASE, WINSDK_VER and HOST_ARCH.

Mar 10 2022, 5:26 PM · Restricted Project, Restricted Project
glandium added a comment to D116709: [CMake][WinMsvc] Fix user passed compiler/linker flags.

The only variables I'm passing: CMAKE_C_COMPILER_TARGET, CMAKE_CXX_COMPILER_TARGET, CMAKE_ASM_COMPILER_TARGET, CMAKE_BUILD_TYPE, LLVM_ENABLE_ASSERTIONS, LLVM_CONFIG_PATH, COMPILER_RT_DEFAULT_TARGET_ONLY, CMAKE_TOOLCHAIN_FILE, LLVM_NATIVE_TOOLCHAIN, MSVC_BASE, WINSDK_BASE, WINSDK_VER, HOST_ARCH.

Mar 10 2022, 5:22 PM · Restricted Project, Restricted Project
Herald added a project to D116709: [CMake][WinMsvc] Fix user passed compiler/linker flags: Restricted Project.

Somehow this change broke cross compiling for 32-bit windows using WinMsvc.cmake. Cmake ends up adding /machine:x64 to the linker flags during one of its first checks, and that fails.

Mar 10 2022, 4:13 PM · Restricted Project, Restricted Project

Feb 22 2022

glandium added a comment to D117611: [Sema] Warn about printf %n on Android and Fuchsia.

This doesn't leave much room to use __attribute__((format(printf))) on custom printf implementations that do support %n on Android does it?

Feb 22 2022, 5:43 PM · Restricted Project

Feb 11 2022

glandium added inline comments to D119546: sanitizer_common: make internal/external headers compatible.
Feb 11 2022, 5:23 PM · Restricted Project

Jan 25 2022

glandium requested review of D118211: Add missing namespace to PPCLinux.cpp.
Jan 25 2022, 8:02 PM · Restricted Project

Dec 27 2021

glandium added a comment to D116263: [lld-macho] Fix alignment of TLV data sections.

@int3 Can you commit this for me?

Dec 27 2021, 8:33 PM · Restricted Project, Restricted Project
glandium updated the diff for D116263: [lld-macho] Fix alignment of TLV data sections.

Addressed comments and added a check that __thread_vars is aligned correctly for the test to be valuable.

Dec 27 2021, 8:31 PM · Restricted Project, Restricted Project

Dec 24 2021

glandium added inline comments to D116263: [lld-macho] Fix alignment of TLV data sections.
Dec 24 2021, 12:06 AM · Restricted Project, Restricted Project
glandium updated the diff for D116263: [lld-macho] Fix alignment of TLV data sections.

Applied clang-format.

Dec 24 2021, 12:03 AM · Restricted Project, Restricted Project

Dec 23 2021

glandium requested review of D116263: [lld-macho] Fix alignment of TLV data sections.
Dec 23 2021, 11:59 PM · Restricted Project, Restricted Project

Dec 13 2021

glandium added a comment to D114638: [ARM] Introduce i8neg and i8pos addressing modes.

FYI, I filed https://github.com/llvm/llvm-project/issues/52681 for a crash that this patch causes.

Dec 13 2021, 9:22 PM · Restricted Project

Dec 3 2021

glandium added a comment to D106585: Fix clang debug info irgen of i128 enums.

Reduced testcase:

enum FrameIID {
  nsBox_id,
  nsIFrame_id,
  nsHTMLFramesetBlankFrame_id,
  nsHTMLFramesetBorderFrame_id,
};
Dec 3 2021, 4:32 AM · Restricted Project, Restricted Project
glandium added a comment to D106585: Fix clang debug info irgen of i128 enums.

So far, what I've found is that in some cases, DIEnumerator::get returns the same node for similar enumerator values in different enums that happen to have the same name and value (even if their bitwidth differs), but sometimes not.
For example, in one compilation I see:

DIEnumerator::get (Name={Data@0x5575ee17ea38="nsNumberControlFrame_id", Length=23}, IsUnsigned: optimized out, Value={U={VAL=50, pVal=0x32}, BitWidth=8}, Context@0x5575e8305da0.pImpl=0x5575e8307230) = 0x55760563a708
DIEnumerator::get (Name={Data@0x5575ee17ea38="nsNumberControlFrame_id", Length=23}, IsUnsigned: optimized out, Value={U={VAL=50, pVal=0x32}, BitWidth=32}, Context@0x5575e8305da0.pImpl=0x5575e8307230) = 0x5576067e7148

In another compilation of the same file with the same flags:

DIEnumerator::get (Name={Data@0x561c659e0cc8="nsNumberControlFrame_id", Length=23}, IsUnsigned: optimized out, Value={U={VAL=50, pVal=0x32}, BitWidth=8}, Context@0x561c5fb68da0.pImpl=0x561c5fb6a230) = 0x561c7ce9dbf8
DIEnumerator::get (Name={Data@0x561c659e0cc8="nsNumberControlFrame_id", Length=23}, IsUnsigned: optimized out, Value={U={VAL=50, pVal=0x32}, BitWidth=32}, Context@0x561c5fb68da0.pImpl=0x561c5fb6a230) = 0x561c7ce9dbf8
Dec 3 2021, 3:58 AM · Restricted Project, Restricted Project

Dec 1 2021

glandium added a comment to D107143: [profile] Fix profile merging with binary IDs.

Not that it matters anymore after D111123 but please note that trunk as of the landing of this (or even trunk before the landing of D111123) says that raw profiles emitted by clang 13.0.0 (which has a backport of this) are malformed.

Dec 1 2021, 7:47 PM · Restricted Project, Restricted Project

Nov 11 2021

glandium added a comment to D106585: Fix clang debug info irgen of i128 enums.

It seems to have been fixed in rG3c47c5ca13b8a502de3272e8105548715947b7a8.

Are you sure you've arrived at the right code review? This change, D106585, modified type information, which is unrelated to the change you linked to. We have also noticed *other* determinism issues (follow up with @hans to learn more), but they aren't related to this change, which is from July.

Nov 11 2021, 3:45 PM · Restricted Project, Restricted Project

Nov 10 2021

glandium added a comment to D106585: Fix clang debug info irgen of i128 enums.

It seems to have been fixed in rG3c47c5ca13b8a502de3272e8105548715947b7a8.

Nov 10 2021, 6:37 PM · Restricted Project, Restricted Project
glandium added a comment to D106585: Fix clang debug info irgen of i128 enums.

It seems to have been fixed in rG3c47c5ca13b8a502de3272e8105548715947b7a8.

Nov 10 2021, 1:38 PM · Restricted Project, Restricted Project
glandium added a comment to D91722: [DebugInfo] Use variadic debug values to salvage BinOps and GEP instrs with non-const operands.

The relanding in 47633af9d4a8b93f50cb711cf23489736e0226f1 broke determinism for Firefox.

Has the non-determinism problem been fixed with the landing of rG3c47c5ca13b8a502de3272e8105548715947b7a8?

Nov 10 2021, 1:29 PM · Restricted Project, debug-info

Nov 4 2021

glandium added a comment to D106585: Fix clang debug info irgen of i128 enums.

This broke determinism when building Firefox.

Nov 4 2021, 4:00 PM · Restricted Project, Restricted Project

Nov 1 2021

glandium added a comment to D91722: [DebugInfo] Use variadic debug values to salvage BinOps and GEP instrs with non-const operands.

The relanding in 47633af9d4a8b93f50cb711cf23489736e0226f1 broke determinism for Firefox.

Nov 1 2021, 3:56 PM · Restricted Project, debug-info

Sep 22 2021

glandium added a comment to D110224: Wrap xar/xar.h include in extern "C" block.

lg.

out of interest, which SDK version has this problem?

Sep 22 2021, 1:36 PM · Restricted Project, Restricted Project
glandium updated the summary of D110224: Wrap xar/xar.h include in extern "C" block.
Sep 22 2021, 3:47 AM · Restricted Project, Restricted Project
glandium requested review of D110224: Wrap xar/xar.h include in extern "C" block.
Sep 22 2021, 12:57 AM · Restricted Project, Restricted Project

Apr 16 2021

glandium added a comment to D96787: Make clangd CompletionModel usable even with non-standard (but supported) layout.

LLVM_EXTERNAL_CLANG_TOOLS_EXTRA_SOURCE_DIR was removed in https://reviews.llvm.org/D58157, so it's unset by default.

Apr 16 2021, 1:00 AM · Restricted Project

Jan 28 2021

glandium added a comment to D95166: Disable rosegment for old Android versions..

Firefox CI is using a custom clang, but uses a NDK otherwise (an old-ish one, clearly older than r22 which is the first that defaults to lld). And we do pass -fuse-ld=bfd at the moment for $reasons.
If clang _really_ wants to assume lld as the linker for android, then it should make using -fuse-ld=somethingelse an error and invoke ld.lld rather than ld, if it doesn't already do that.

Jan 28 2021, 4:23 PM · Restricted Project

Jan 27 2021

glandium added a comment to D95166: Disable rosegment for old Android versions..

"Android only supports lld" might be true now, but it hasn't always been true, which means the change is not backwards compatible with versions of the NDK that don't use lld. Also, -fuse-ld is still a valid flag that can allow to use a different linker than lld.

Jan 27 2021, 4:23 PM · Restricted Project

Nov 26 2020

glandium added a comment to D72420: [LoopRotate] Add support for rotating loops with switch exit.

FWIW, when applying this patch on a LLVM10-based rustc, the rustc build fails with:

PHI node has multiple entries for the same basic block with different incoming values!
  %35 = phi { i64*, i32 }* [ %83, %77 ], [ %84, %77 ], [ %33, %2 ]
label %77
  %83 = getelementptr { i64*, i32 }, { i64*, i32 }* %35, i64 -1
  %84 = getelementptr { i64*, i32 }, { i64*, i32 }* %35, i64 -1
in function _ZN80_$LT$alloc..vec..Vec$LT$T$GT$$u20$as$u20$alloc..vec..SpecExtend$LT$T$C$I$GT$$GT$9from_iter17h62b0d0798a862a64E
LLVM ERROR: Broken function found, compilation aborted!
error: could not compile `rustc_typeck`.
Nov 26 2020, 5:32 PM · Restricted Project

Feb 7 2019

glandium added a comment to D57636: [COFF, ARM64] Fix types for _ReadStatusReg, _WriteStatusReg.

LGTM. Do you want me to commit this for you?

Feb 7 2019, 5:03 PM · Restricted Project, Restricted Project
glandium added a comment to D57636: [COFF, ARM64] Fix types for _ReadStatusReg, _WriteStatusReg.

@efriedma can you take another look? Ideally, this should be backported to the release_80 branch, so that would need to be landed asap.

Feb 7 2019, 4:53 PM · Restricted Project, Restricted Project

Feb 4 2019

glandium updated the diff for D57636: [COFF, ARM64] Fix types for _ReadStatusReg, _WriteStatusReg.

Updated EmitAArch64BuiltinExpr per https://reviews.llvm.org/D57636#1383751 and the testcase per https://reviews.llvm.org/D57636#1384348

Feb 4 2019, 7:01 PM · Restricted Project, Restricted Project
glandium added a comment to D57636: [COFF, ARM64] Fix types for _ReadStatusReg, _WriteStatusReg.

(It should be possible to check that we aren't inserting incorrect truncation/extension operations in the IR.)

Feb 4 2019, 4:29 PM · Restricted Project, Restricted Project

Feb 1 2019

glandium added a comment to D51204: [COFF, ARM64] Add MS intrinsics: __getReg, _ReadStatusReg, _WriteStatusReg.

Opened D57636

Feb 1 2019, 8:00 PM
glandium created D57636: [COFF, ARM64] Fix types for _ReadStatusReg, _WriteStatusReg.
Feb 1 2019, 7:59 PM · Restricted Project, Restricted Project
glandium added a comment to D51204: [COFF, ARM64] Add MS intrinsics: __getReg, _ReadStatusReg, _WriteStatusReg.

Will abandon this patch since I have implementations of these which I will upstream soon.

Just to link up the reviews: these landed in D52838 and D53115. (Thanks @mgrang!)

Feb 1 2019, 2:52 PM

Jan 8 2019

glandium created D56475: Don't require a null terminator when loading objects.
Jan 8 2019, 9:37 PM

Nov 28 2018

glandium added a comment to D54891: [RFC] Checking inline assembly for validity.

I can provide a full log of the 4831 warnings marked -Winline-asm during a Firefox for ARM Android build, if you're interested.

Yes, it would be helpful if you could send me that output (with the latest patch applied).

Nov 28 2018, 11:57 PM

Nov 27 2018

glandium added a comment to D54891: [RFC] Checking inline assembly for validity.

Correction, the mpi_arm.c ones seem to be caught. However, warnings are also being emitted for assembly post substitution. Example:

Nov 27 2018, 6:23 PM

Nov 26 2018

glandium added a comment to D54891: [RFC] Checking inline assembly for validity.

Assuming removing the variadicOpsAreDefs test completely (since I couldn't figure where it came from) still yields something that mostly works, it appears not to catch when the assembly writes to input operands. Examples of real code that had the problem:

Nov 26 2018, 6:18 PM
glandium added inline comments to D54891: [RFC] Checking inline assembly for validity.
Nov 26 2018, 3:11 PM

Aug 28 2018

glandium added inline comments to D47656: Show choices of multiple-choice options in `ld.lld --help` message..
Aug 28 2018, 6:08 AM

Nov 7 2017

glandium updated the diff for D39717: Always use prctl(PR_SET_PTRACER).

Updated wording.

Nov 7 2017, 2:10 PM
glandium created D39717: Always use prctl(PR_SET_PTRACER).
Nov 7 2017, 1:00 AM