Page MenuHomePhabricator

nsz (Szabolcs Nagy)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 9 2018, 8:54 AM (141 w, 2 d)

Recent Activity

Aug 11 2020

nsz added a comment to D80791: [AArch64] Generate .note.gnu.property based on module flags..

it is not useful to have a bti annotated function unless everything else is bti compatible too: it is all or nothing per elf module.

This is false. Some functions in an elf module could be in a guarded region, some in a non-guarded region. Some function may always
be called in a "BTI-safe" way, which may be unknown to the compiler.

Right now the elf and all of the text sections considered BTI enabled or not. The dynamic linkers/loaders can't support this
use case without additional information to be encoded somewhere (and specified). To support such we need to consider grouping/align to page
boundaries these functions in the linker because BTI could be controlled by flags in PTE.
With the current spec this usecase is not supported in this way. The user have to link the BTI protected code into another elf.
Side note: The force-bti linker option can't work with half BTI enabled objects.

I suppose this is valid for typical Linux-based systems today.

Is it valid in general, across the whole spectre of operating systems or for bare-metal targets?

Guess not.

Aug 11 2020, 8:14 AM · Restricted Project, Restricted Project
nsz added a comment to D80791: [AArch64] Generate .note.gnu.property based on module flags..
In D80791#2209624, @nsz wrote:
  • it is not useful to have a bti annotated function unless everything else is bti compatible too: it is all or nothing per elf module.

This is false. Some functions in an elf module could be in a guarded region, some in a non-guarded region. Some function may always
be called in a "BTI-safe" way, which may be unknown to the compiler.

Aug 11 2020, 7:27 AM · Restricted Project, Restricted Project
nsz added a comment to D80791: [AArch64] Generate .note.gnu.property based on module flags..

I would prefer to avoid the situation where the markings of two otherwise identical files were different,
depending on how the files were produced, no matter if it was a common or a special case.

Aug 11 2020, 5:00 AM · Restricted Project, Restricted Project

Aug 10 2020

nsz added a comment to D80791: [AArch64] Generate .note.gnu.property based on module flags..
In D80791#2206853, @nsz wrote:

i think that cannot work.

the implementation is free to inject arbitrary code into
user code so if the user does not tell the implementation
that it wants the entire tu to be bti safe then non-bti
code can end up in there. (e.g. ctor of an instrumentation
that is not realated to any particular function with the
bti marking)

Certainly, there are cases it won't work, but there are definitely
cases where it *can* work. Whatever the implementation does
should be a deterministic consequence of implementing the relevant
language standards together with implementation-defined behaviour,
command-line options and language extensions (e..g attributes).

Certainly I don't expect C++ ctorts/dtors in C code and gcov or
sanitiser calls if I haven't given relevant -fprofile-whatever/-fsanitize=whatever
options. In that sense, the implementation cannot do whatever
it pleases, it is constrained to a range of behaviours one can reason about.

Aug 10 2020, 9:22 AM · Restricted Project, Restricted Project
nsz updated subscribers of D80791: [AArch64] Generate .note.gnu.property based on module flags..

The 08/10/2020 10:03, Momchil Velikov via Phabricator wrote:

chill added a comment.

In D80791#2196598 https://reviews.llvm.org/D80791#2196598, @nsz wrote:

the assumption is that the intended branch protection is implied via cmdline flags for the tu and function attributes are only used in source code for some hack.

I don't share this assumption. I find it just as valid to control the PAC/BTI with things like:

#ifdef ENABLE_BTI
#define BTI_FUNC __attribute__((target("branch-protection=bti")))
#else
#define BTI_FUNC

BTI_FUNC void foo() { ...
BTI_FUNC int bar() { ...

without using any command-line option other than -DENABLE_BTI=1.

Aug 10 2020, 7:49 AM · Restricted Project, Restricted Project

Aug 5 2020

nsz added a comment to D80791: [AArch64] Generate .note.gnu.property based on module flags..

the gcc behaviour is not exactly ideal, but it's better if llvm is compatible with it or fix gcc if something is broken there.

Aug 5 2020, 8:49 AM · Restricted Project, Restricted Project

Jan 24 2020

nsz added a comment to D72222: [Driver][CodeGen] Add -fpatchable-function-entry=N[,0].

A -DLLVM_ENABLE_ASSERTIONS=on build is required to trigger the assertion failure. My make ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- HOSTGCC=gcc CC=~/llvm/ReleaseAssert/bin/clang LD=~/llvm/ReleaseAssert/bin/ld.lld O=/tmp/arm64 allmodconfig all -j 30 build succeeded. Now I will be in favor of pushing the bugfix to release/10.x .

Not clear about -fpatchable-function-entry=N,M where M>0 (D73070, D73071, D73072). For completeness, I'd like them to be included in release/10.x so we will not have a clang 10 that does not work with M>0.

For BTI c issue, GCC has several releases that do not work with -mbranch-protection=bti. The Linux kernel has to develop some mechanism to detect the undesirable placement of bti c, if there are -mbranch-protection=bti users. So I don't think that inconsistency in clang 10.0.0 with GCC will be a problem.

Jan 24 2020, 2:05 AM · Restricted Project

Jul 24 2019

nsz added a comment to D64930: [ELF][AArch64] Allow PT_LOAD to have overlapping p_offset ranges.

Will be worth mentioning that this is dependent on D64906 and D64903, I could guess at D64903 but basic-aarch64.s didn't apply cleanly without D64903. I successfully did a 2 stage bootstrap of clang with ninja check-all on a native aarch64 machine and made a quick test to check the TP relative relocation. I struggled a bit to understand the description and the comments though. In particular the use of GAP_ABOVE_TP instead of TCB_SIZE, it also wasn't clear where the alignment calculation had come from. I've made a few suggestions.

Jul 24 2019, 2:14 AM · Restricted Project

Jul 17 2019

nsz added a comment to D63863: [ARM] Make coprocessor number restrictions consistent..

Would some kind of architectural extension be a reasonable solution? -march=armv8+oldcoprocessors, or some such? That way -march=armv8 continues to diagnose things that actually are illegal in the architecture you asked for.

Jul 17 2019, 6:13 AM · Restricted Project
nsz added a comment to D63863: [ARM] Make coprocessor number restrictions consistent..

Hmmm. This surely can't be the first time a case like this has come up. What's the usual solution in other similar situations, when you want to include code for mutually incompatible architectures in the same object because you're going to test at run time which one to execute?

Jul 17 2019, 1:53 AM · Restricted Project

May 15 2019

nsz added a comment to D61824: [ARM][AArch64] Overalign .tbss (Android Bionic hack) to avoid p_vaddr%p_align!=0.

I think that basically works. It would add a TLS segment to every Android executable, even those that aren't using ELF TLS:

  • If the placeholder symbol has size 1 and 64-byte alignment, then every Android executable (targeting Q(29) and up) would have a PT_TLS segment of 64-byte size and alignment with gold or lld.
  • If the placeholder symbol in .tdata has size 0 instead, then ld.bfd omits the .tdata section from the output if there aren't any other non-empty .tdata sections. This is a problem if there are other TLS sections (.tbss). Android is switching to lld, though, so maybe it's OK. (The NDK still defaults to bfd and gold.)
  • If the placeholder symbol has a size of 48 bytes and 1-byte alignment, then libc.so can't use alignment to detect whether space was reserved. I can think of a couple ways for space to not be reserved:
    • Using an NDK that's too old
    • Using a too-old NDK API level, assuming this placeholder symbol is omitted when targeting old versions of Bionic (because Q(29) is currently rejecting an underaligned TLS segment...)
May 15 2019, 4:29 AM · Restricted Project

May 13 2019

Herald added a project to D53906: [ARM][AArch64] Increase TLS alignment to reserve space for Android's TCB: Restricted Project.

i think this should be fixed in the tools/runtimes that assume fixed tls offset abi that conflict with the elf tls abi.

May 13 2019, 9:27 AM · Restricted Project
nsz added a comment to D61824: [ARM][AArch64] Overalign .tbss (Android Bionic hack) to avoid p_vaddr%p_align!=0.

i think glibc is right (comes up with a tlsoffset%p_align == p_vaddr%p_align for the module) and musl and other implementations are wrong (they may not get the tlsoffset right in this case: it seems musl always computes tlsoffset%p_align==0 on tls variant 1 targets).

May 13 2019, 8:48 AM · Restricted Project

Jan 26 2018

nsz added inline comments to D40134: [asan] Add support for AArch64 ILP32.
Jan 26 2018, 8:05 AM · Restricted Project

Jan 12 2018

nsz added a comment to D40673: Add _Float128 as alias to __float128 to enable compilations on Fedora27/glibc2-26.

as this patch is committed clang is broken (cannot use glibc headers)

This patch was supposed to *fix* compatibility with the glibc headers. If it doesn't, we should clearly revert it... but we need to figure out what we need to do to be compatible first.

From what I can see on this thread, we *must* define _Float128 on x86-64 Linux, and we *must not* define _Float128 for any other glibc target. Is that correct? Or does it depend on the glibc version?

Jan 12 2018, 12:18 PM · Restricted Project

Jan 11 2018

nsz added a comment to D40673: Add _Float128 as alias to __float128 to enable compilations on Fedora27/glibc2-26.

also note that there is less than 3 weeks until glibc-2.27 is released, if the headers need a fix for clang then say so quickly

Jan 11 2018, 3:24 AM · Restricted Project
nsz added a comment to D40673: Add _Float128 as alias to __float128 to enable compilations on Fedora27/glibc2-26.

if clang wants to provide _Float128 then the f128 constant suffix (specified by TS18661-3) and builtin_inff128, builtin_nanf128, builtin_nansf128, builtin_huge_valf128 (gcc builtins required by math.h) need to be supported too.

Jan 11 2018, 3:13 AM · Restricted Project