[PATCH] D80791: [AArch64] Generate .note.gnu.property based on module flags.

[Szabolcs Nagy via Phabricator via cfe-commits]

nsz added a comment.

In D80791#2210207 , @chill wrote:

> In D80791#2210124 , @danielkiss 
> 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.
>>
>> 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.

the linker may or may not need to generate code (PLT is just one example) and 
the current abi is designed such that it is an elf module level decision if 
that code is bti compatible or not.

this is why i said that the current abi does not support mixed bti and non-bti 
code, however the user can do this if lies to the linker that everything is bti 
compatible so the linker generates stub code accordingly.

i don't see how baremetal can get away here: this is elf abi.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80791/new/

https://reviews.llvm.org/D80791

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80791: [AArch64] Generate .note.gnu.property based on module flags.

[Szabolcs Nagy via Phabricator via cfe-commits]

nsz added a comment.

In D80791#2209703 , @chill wrote:

> 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.

the current design is per elf module, so the non-guarded things have to go into 
a different elf module (and thus different tu).

i think the only way the current abi supports mixing bti and non-bti code is if 
all the linker inputs to the elf module are marked as bti compatible and then 
the user explicitly unprotects some region at runtime, i.e. bti is still all or 
nothing per elf module, but a user might want to do some hack and turn bti off 
in some places.

> With my proposal to derive marking from function attributes, as well as from 
> command-line
> everything above will still work in the (arguably) most common case that we 
> expect - users just using
> command line.
>
> I'm proposing to be strict and cover a few corner case where the command-line 
> only approach produces bogus results.

ok i think deriving the marking in the absence of command-line option works, 
but it's not something users can rely on and not what gcc does.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80791/new/

https://reviews.llvm.org/D80791

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80791: [AArch64] Generate .note.gnu.property based on module flags.

[Szabolcs Nagy via Phabricator via cfe-commits]

nsz added a comment.

In D80791#2207203 , @chill wrote:

> 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.

i don't see why it is desirable to silently get marking on an object file if 
function definitions happen to be bti compatible in it:

- compiler cannot reliably do this (e.g. bti incompatible inline asm).
- some users don't want the marking: not all linkers support it so it can cause 
unexpected breakage.
- most users (all?) want the marking reliably (not opportunistically), but 
function annotations are fragile (can depend on optimizations and code outside 
of user control).
- 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.
- but a compiler cannot diagnose if only some functions have the annotation (we 
don't have a cflag for it) so even if the compiler tried to add the marking 
silently users cannot rely on it: it's too easy to drop the marking and no way 
to debug such failure.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80791/new/

https://reviews.llvm.org/D80791

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80791: [AArch64] Generate .note.gnu.property based on module flags.

[Szabolcs Nagy via Phabricator via cfe-commits]

nsz added a comment.

In D80791#2206933 , @chill wrote:

> 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.

i think it's a bad idea to use function level
attributes to control what markings we attach to
translation units and i would prefer to only add
markings to object files when the compiler is
asked to do so per tu.

i dont want to see source code changes
to enable bti, that should be only needed
for some special case that's way out of
standard conform code.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80791/new/

https://reviews.llvm.org/D80791

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80791: [AArch64] Generate .note.gnu.property based on module flags.

[Szabolcs Nagy via Phabricator via cfe-commits]

nsz added a comment.

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.

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. a common reason for such hack is to disable bti somewhere but still keep 
the bti elf marking. (if the intention was to mark the code non-bti compatible 
then just dont compile it with bti, using a non-portable  bti specfic function 
attribute would not work well for such use anyway)

now this may not work well with lto when functions can come from different 
places and the function attributes encode how those were compiled. so hopefully 
there is something that preserves the flags of the translation units and 
explicitly specified function attributes can be treated separately (such that 
they dont affect elf markings).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80791/new/

https://reviews.llvm.org/D80791

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72222: [Driver][CodeGen] Add -fpatchable-function-entry=N[,0]

[Szabolcs Nagy via Phabricator via cfe-commits]

nsz added a comment.

In D7#1837536 , @MaskRay wrote:

> 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.


note that the gcc fix will be in the gcc-10 release (it got accepted),
and soon backported to gcc-9 (the first release with bti support).
(only aarch64 bti was fixed, that's what affected linux)

the gcc fix keeps bti outside of the patch area in the M=0 case,
which makes the current linux *runtime* patching code simpler.

the choice was made because this is important if there is a mix
of bti and non-bti functions which can happen if

1. compiler detects not_called_directly(f) and omits bti or
2. individual attributes on functions turn on/off bti

with the llvm patch the *runtime* code would have to detect
which patch area has bti and which not to skip over. (fine for
future M>0 usage, but current M=0 case should not be complex,
i.e. linux will have to disable bti+ftrace on llvm if this patch is
accepted until it has the complex runtime code. and the patch
may make static detection of the compiler behaviour harder as
it fails differently than old gcc-9).

i did not look deeply into llvm but i'd expect some hook for
printing the function label, which can print the bti before
the patch area based on some global flag (an ugly hack, but
allows consistent behaviour across the compilers)

i hope this makes sense.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D7/new/

https://reviews.llvm.org/D7



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40673: Add _Float128 as alias to __float128 to enable compilations on Fedora27/glibc2-26

[Szabolcs Nagy via Phabricator via cfe-commits]

nsz added a comment.

In https://reviews.llvm.org/D40673#973638, @efriedma wrote:

> > 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?


it is not clear to me from the original bug report what "fedora 27 workloads" 
break because of the lack of _Float128 type on x86.  The glibc headers refer to 
_Float128, but normal include should not break unless the compiler claims to be 
>=gcc-7 or somebody explicitly requested the _Float128 support.

if clang defines _Float128 then the headers "work" on x86 as in simple math.h 
include is not broken, but some macro definitions will use the f128 const 
suffix (e.g. FLT128_MAX) which won't cause much breakage now but in the future 
users will want to know whether they can use these macros or not.

so either clang have to introduce all these features that are used by glibc 
together or provide ways for users (and glibc) to figure out what is supported 
and what isn't.

on non-x86 targets a glibc fix can solve the problem such that they "work" on 
the same level as x86, i.e. no immediate build breakage on most code, but some 
features are not supported. i think that's the right way forward, but it's not 
clear if anybody has time to do the header changes in glibc before release (it 
has to be tested on several targets).

if glibc is released as is today then those non-x86 targets don't want a 
_Float128 definition in clang-6 that breaks any math.h include on a glibc-2.27 
system.

> (We have a bit of time before the 6.0 release, so we can adjust the behavior 
> here to make it work.  We probably don't want to try to add full _Float128 
> support on the branch, though.)




Repository:
  rC Clang

https://reviews.llvm.org/D40673



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40673: Add _Float128 as alias to __float128 to enable compilations on Fedora27/glibc2-26

[Szabolcs Nagy via Phabricator via cfe-commits]

nsz added a comment.

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

i opened https://sourceware.org/bugzilla/show_bug.cgi?id=22700 but it needs 
attention from some clang developers,
in particular there is no way to check if the compiler has _Float128 type 
defined but no working f128 suffix in the headers.


Repository:
  rC Clang

https://reviews.llvm.org/D40673



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40673: Add _Float128 as alias to __float128 to enable compilations on Fedora27/glibc2-26

[Szabolcs Nagy via Phabricator via cfe-commits]

nsz added a comment.

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.

as this patch is committed clang is broken (cannot use glibc headers) on any 
target where long double is quad precision (aarch64, mips64, s390, sparc64, 
riscv64) even if glibc fixes the >=gcc-7 check to something that works for 
clang: either the _Float128 typedef conflicts with the definition by clang or 
the suffixes/builtins are missing.


Repository:
  rC Clang

https://reviews.llvm.org/D40673



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits