Re: SSA Iterators
On Thu, 30 Jan 2020, 05:44 Nicholas Krause wrote: > > Greetings, > > I was looking into starting to cleaning up the SSA trees for various > reasons and iterators > seem to be the easiest to do. I searched the list to see if someone > mentioned it before > and I ran across this: > https://gcc.gnu.org/ml/gcc-patches/2019-10/msg02031.html > > If your trying to get a second ++ working then why not override it > prefix as your > just doing it for postfix and those can be separate. No no no. No. > Its not ideal to > have two > different versions for prefix and postfix in terms of overloading but it > may be > the simplest solution here. No, making pre-increment and post-incrememt so different things (i.e. iterate over different ranges) is a horrible idea. That's the kind of thing that gives operator overloading a bad name. Overloaded operators should do the thing you expect them to. They should not be used to hide a non-obvious action behind an apparently simple syntax. I would suggest avoiding "smart" iterators that contain all the state and know their own end point. Instead create a type that holds all the state and has begin/end member functions which return an iterator that refers to the state. And have the iterator dereference to some other object that has the state for the second level of iteration, with its own begin/end members returning the second iterator type. That would end up looking like: imm_use_stmt_range r (SSAVAR); for (imm_use_stmt_iter it = r.begin (); it != r.end (); ++it) for (imm_use_on_stmt_iter it2 = it->begin (); it2 != it->end (); ++it2) ; At some point when we move to C++11 or later that could become: imm_use_stmt_range r (SSAVAR); for (auto& stmt : r) for (auto& on_stmt : *stmt) ;
Re: [musl] musl, glibc and ideal place for __stack_chk_fail_local
On Sat, Jan 25, 2020 at 10:54:24AM -0500, Rich Felker wrote: > > To support smash stack protection gcc emits __stack_chk_fail > > calls on all targets. On top of that gcc emits __stack_chk_fail_local > > calls at least on i386 and powerpc: (Only on 32-bit -fPIC -msecure-plt, for Power). > There is a half-serious proposal to put it in crti.o which is always > linked too, but that seems like an ugly hack to me... Not *very* ugly, but it would be very effective, and no real downsides to it (or do you see something?) > > My understanding of requirements for libc that exposes ssp support: > > - __stack_chk_fail is implemented as a default symbol > > - __stack_chk_fail_local is implemented as a local symbol to avoid PLT. > > (Why is it important? To avoid use of potentially already broken stack?) > > Because performance cost of -fstack-protector would go from 1-2% up to > 5-10% on i386 and other archs where PLT contract requires a GOT > register, since loading the GOT register is expensive > (__x86.get_pc_thunk.* thunk itself is somewhat costly, and you throw > away one of only a small number of available registers, increasing > register pressure and hurting codegen). On Power it is just the setting up itself that is costly (in the config where we have this _local thing). > Absolutely not. libssp is unsafe and creates new vulns/attack surface > by doing introspective stuff after the process is already *known to > be* in a compromised state. It should never be used. musl's > __stack_chk_fail is safe and terminates immediately. Some implementations even print strings from the stack, it can be worse ;-) > Ideally, though, GCC would just emit the termination inline (or at > least have an option to do so) rather than calling __stack_chk_fail or > the local version. This would additionally harden against the case > where the GOT is compromised. Yeah, but how to terminate is system-specific, it's much easier to punt this job to the libc to do ;-) Open a GCC PR for this please? Segher
Re: [musl] musl, glibc and ideal place for __stack_chk_fail_local
On Thu, Jan 30, 2020 at 06:33:51AM -0600, Segher Boessenkool wrote: > On Sat, Jan 25, 2020 at 10:54:24AM -0500, Rich Felker wrote: > > > To support smash stack protection gcc emits __stack_chk_fail > > > calls on all targets. On top of that gcc emits __stack_chk_fail_local > > > calls at least on i386 and powerpc: > > (Only on 32-bit -fPIC -msecure-plt, for Power). Right, but musl only supports the secure-plt ABI. > > There is a half-serious proposal to put it in crti.o which is always > > linked too, but that seems like an ugly hack to me... > > Not *very* ugly, but it would be very effective, and no real downsides > to it (or do you see something?) Well either the thunk has to be written in asm per-arch, or some ld -r magic (which is fragile and something I don't want musl to depend on since I know users will someday hit breakage and rightfully blame us for using ld -r) to merge an asm source and C source. Or perhaps the existing crti.s content could be moved to file-scope __asm__ included in the C source file...that might be ok? > > > My understanding of requirements for libc that exposes ssp support: > > > - __stack_chk_fail is implemented as a default symbol > > > - __stack_chk_fail_local is implemented as a local symbol to avoid PLT. > > > (Why is it important? To avoid use of potentially already broken stack?) > > > > Because performance cost of -fstack-protector would go from 1-2% up to > > 5-10% on i386 and other archs where PLT contract requires a GOT > > register, since loading the GOT register is expensive > > (__x86.get_pc_thunk.* thunk itself is somewhat costly, and you throw > > away one of only a small number of available registers, increasing > > register pressure and hurting codegen). > > On Power it is just the setting up itself that is costly (in the config > where we have this _local thing). I think it'd be the same. If a function otherwise has no reason to access global data or calls though PLT, it can avoid the cost of finding the GOT and spending a fixed register on it. But possibility of having to call __stack_chk_fail makes *every* (stack-protected) function need to be able to make calls thru PLT, and thus introduces this cost to every function. > > Absolutely not. libssp is unsafe and creates new vulns/attack surface > > by doing introspective stuff after the process is already *known to > > be* in a compromised state. It should never be used. musl's > > __stack_chk_fail is safe and terminates immediately. > > Some implementations even print strings from the stack, it can be worse ;-) :-) > > Ideally, though, GCC would just emit the termination inline (or at > > least have an option to do so) rather than calling __stack_chk_fail or > > the local version. This would additionally harden against the case > > where the GOT is compromised. > > Yeah, but how to terminate is system-specific, it's much easier to punt > this job to the libc to do ;-) My ideas was __builtin_trap, although a slightly more hardened version (that might make users unhappy? :) is inlining a syscall to sigprocmask to mask SIGILL/SIGSEGV before the trapping instruction so that termination occurs regardless of whether there's a signal handler installed. > Open a GCC PR for this please? Filed as https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93509 Rich
Re: [musl] musl, glibc and ideal place for __stack_chk_fail_local
On Thu, Jan 30, 2020 at 08:37:40AM -0500, Rich Felker wrote: > On Thu, Jan 30, 2020 at 06:33:51AM -0600, Segher Boessenkool wrote: > > On Sat, Jan 25, 2020 at 10:54:24AM -0500, Rich Felker wrote: > > > > To support smash stack protection gcc emits __stack_chk_fail > > > > calls on all targets. On top of that gcc emits __stack_chk_fail_local > > > > calls at least on i386 and powerpc: > > > > (Only on 32-bit -fPIC -msecure-plt, for Power). > > Right, but musl only supports the secure-plt ABI. Sure, it is the modern one. Still only for 32-bit -fPIC for musl, too. > > > There is a half-serious proposal to put it in crti.o which is always > > > linked too, but that seems like an ugly hack to me... > > > > Not *very* ugly, but it would be very effective, and no real downsides > > to it (or do you see something?) > > Well either the thunk has to be written in asm per-arch, or some ld -r > magic (which is fragile and something I don't want musl to depend on > since I know users will someday hit breakage and rightfully blame us > for using ld -r) to merge an asm source and C source. Or perhaps the > existing crti.s content could be moved to file-scope __asm__ included > in the C source file...that might be ok? At least for powerpc, the existing crti.s gets stuff inserted after (in both functions), and then closed off by crtn.s -- not something you want to do in C :-) GCC can just say to also use extra crti files -- see STARTFILE_SPEC. Many platforms do that already. > > On Power it is just the setting up itself that is costly (in the config > > where we have this _local thing). > > I think it'd be the same. We don't have a shortage of usable registers, that's what I was getting at. All the other arguments are similar, sure. > > > Absolutely not. libssp is unsafe and creates new vulns/attack surface > > > by doing introspective stuff after the process is already *known to > > > be* in a compromised state. It should never be used. musl's > > > __stack_chk_fail is safe and terminates immediately. > > > > Some implementations even print strings from the stack, it can be worse ;-) > > :-) It wasn't a joke, unfortunately. > > > Ideally, though, GCC would just emit the termination inline (or at > > > least have an option to do so) rather than calling __stack_chk_fail or > > > the local version. This would additionally harden against the case > > > where the GOT is compromised. > > > > Yeah, but how to terminate is system-specific, it's much easier to punt > > this job to the libc to do ;-) > > My ideas was __builtin_trap, although a slightly more hardened version > (that might make users unhappy? :) is inlining a syscall to > sigprocmask to mask SIGILL/SIGSEGV before the trapping instruction so > that termination occurs regardless of whether there's a signal handler > installed. I think we should make this a separate RTL pattern? Or a (noreturn) libgcc function? Anyway, let's talk in the PR :-) > > Open a GCC PR for this please? > > Filed as https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93509 Thanks! Segher
Re: SSA Iterators
On 1/30/20 2:59 AM, Jonathan Wakely wrote: On Thu, 30 Jan 2020, 05:44 Nicholas Krause wrote: Greetings, I was looking into starting to cleaning up the SSA trees for various reasons and iterators seem to be the easiest to do. I searched the list to see if someone mentioned it before and I ran across this: https://gcc.gnu.org/ml/gcc-patches/2019-10/msg02031.html If your trying to get a second ++ working then why not override it prefix as your just doing it for postfix and those can be separate. No no no. No. Its not ideal to have two different versions for prefix and postfix in terms of overloading but it may be the simplest solution here. No, making pre-increment and post-incrememt so different things (i.e. iterate over different ranges) is a horrible idea. That's the kind of thing that gives operator overloading a bad name. Overloaded operators should do the thing you expect them to. They should not be used to hide a non-obvious action behind an apparently simple syntax. I would suggest avoiding "smart" iterators that contain all the state and know their own end point. Instead create a type that holds all the state and has begin/end member functions which return an iterator that refers to the state. And have the iterator dereference to some other object that has the state for the second level of iteration, with its own begin/end members returning the second iterator type. That would end up looking like: imm_use_stmt_range r (SSAVAR); for (imm_use_stmt_iter it = r.begin (); it != r.end (); ++it) for (imm_use_on_stmt_iter it2 = it->begin (); it2 != it->end (); ++it2) ; At some point when we move to C++11 or later that could become: imm_use_stmt_range r (SSAVAR); for (auto& stmt : r) for (auto& on_stmt : *stmt) ; That's a good goal to aim for with all GCC "iterators," including those behind the legacy FOREACH_XXX macros. I posted a patch for one of these in 2018 but it was too late in the development cycle and I didn't get back to in in 2019: https://gcc.gnu.org/ml/gcc-patches/2018-11/msg01549.html If you're working on a general cleanup in this are please consider these as well. Martin
Re: SSA Iterators
On January 30, 2020 5:05:09 PM GMT+01:00, Martin Sebor wrote: >On 1/30/20 2:59 AM, Jonathan Wakely wrote: >> On Thu, 30 Jan 2020, 05:44 Nicholas Krause wrote: >>> >>> Greetings, >>> >>> I was looking into starting to cleaning up the SSA trees for various >>> reasons and iterators >>> seem to be the easiest to do. I searched the list to see if someone >>> mentioned it before >>> and I ran across this: >>> https://gcc.gnu.org/ml/gcc-patches/2019-10/msg02031.html >>> >>> If your trying to get a second ++ working then why not override it >>> prefix as your >>> just doing it for postfix and those can be separate. >> >> No no no. >> >> No. >> >>> Its not ideal to >>> have two >>> different versions for prefix and postfix in terms of overloading >but it >>> may be >>> the simplest solution here. >> >> No, making pre-increment and post-incrememt so different things (i.e. >> iterate over different ranges) is a horrible idea. That's the kind of >> thing that gives operator overloading a bad name. Overloaded >operators >> should do the thing you expect them to. They should not be used to >> hide a non-obvious action behind an apparently simple syntax. >> >> >> I would suggest avoiding "smart" iterators that contain all the state >> and know their own end point. Instead create a type that holds all >the >> state and has begin/end member functions which return an iterator >that >> refers to the state. And have the iterator dereference to some other >> object that has the state for the second level of iteration, with its >> own begin/end members returning the second iterator type. That would >> end up looking like: >> >>imm_use_stmt_range r (SSAVAR); >>for (imm_use_stmt_iter it = r.begin (); it != r.end (); ++it) >> for (imm_use_on_stmt_iter it2 = it->begin (); it2 != it->end (); >++it2) >>; >> >> At some point when we move to C++11 or later that could become: >> >>imm_use_stmt_range r (SSAVAR); >>for (auto& stmt : r) >> for (auto& on_stmt : *stmt) >>; > >That's a good goal to aim for with all GCC "iterators," including >those behind the legacy FOREACH_XXX macros. I posted a patch for >one of these in 2018 but it was too late in the development cycle >and I didn't get back to in in 2019: > https://gcc.gnu.org/ml/gcc-patches/2018-11/msg01549.html > >If you're working on a general cleanup in this are please consider >these as well. We should definitely aim for a standard way, adding yet another one doesn't help. Which is why I was trying to tackle the more difficult one(s) first. Most of the iterators should be trivial to convert to whatever style we settle on. Richard. >Martin
Re: SSA Iterators
On 1/30/20 11:25 AM, Richard Biener wrote: On January 30, 2020 5:05:09 PM GMT+01:00, Martin Sebor wrote: On 1/30/20 2:59 AM, Jonathan Wakely wrote: On Thu, 30 Jan 2020, 05:44 Nicholas Krause wrote: Greetings, I was looking into starting to cleaning up the SSA trees for various reasons and iterators seem to be the easiest to do. I searched the list to see if someone mentioned it before and I ran across this: https://gcc.gnu.org/ml/gcc-patches/2019-10/msg02031.html If your trying to get a second ++ working then why not override it prefix as your just doing it for postfix and those can be separate. No no no. No. Its not ideal to have two different versions for prefix and postfix in terms of overloading but it may be the simplest solution here. No, making pre-increment and post-incrememt so different things (i.e. iterate over different ranges) is a horrible idea. That's the kind of thing that gives operator overloading a bad name. Overloaded operators should do the thing you expect them to. They should not be used to hide a non-obvious action behind an apparently simple syntax. I would suggest avoiding "smart" iterators that contain all the state and know their own end point. Instead create a type that holds all the state and has begin/end member functions which return an iterator that refers to the state. And have the iterator dereference to some other object that has the state for the second level of iteration, with its own begin/end members returning the second iterator type. That would end up looking like: imm_use_stmt_range r (SSAVAR); for (imm_use_stmt_iter it = r.begin (); it != r.end (); ++it) for (imm_use_on_stmt_iter it2 = it->begin (); it2 != it->end (); ++it2) ; At some point when we move to C++11 or later that could become: imm_use_stmt_range r (SSAVAR); for (auto& stmt : r) for (auto& on_stmt : *stmt) ; That's a good goal to aim for with all GCC "iterators," including those behind the legacy FOREACH_XXX macros. I posted a patch for one of these in 2018 but it was too late in the development cycle and I didn't get back to in in 2019: https://gcc.gnu.org/ml/gcc-patches/2018-11/msg01549.html If you're working on a general cleanup in this are please consider these as well. We should definitely aim for a standard way, adding yet another one doesn't help. Which is why I was trying to tackle the more difficult one(s) first. Most of the iterators should be trivial to convert to whatever style we settle on. Richard. Richard, I do recall there was some agreement on moving to C++11 last year so for each loops should be fine. I'm only aware of the SSA iterators as I was just researching the SSA code. Not sure of the others as those were the other ones mentioned in the GCC manual from memory. In addition the STL mentions const, standard and reverse iterators. I'm assuming we only need standard but perhaps const versions may be useful as well. Regards, Nick Martin
Confused about code/comment in tree.c:build2
Hi, In tree.c:build2 there is following code/comment: if ((code == MINUS_EXPR || code == PLUS_EXPR || code == MULT_EXPR) && arg0 && arg1 && tt && POINTER_TYPE_P (tt) /* When sizetype precision doesn't match that of pointers we need to be able to build explicit extensions or truncations of the offset argument. */ && TYPE_PRECISION (sizetype) == TYPE_PRECISION (tt)) gcc_assert (TREE_CODE (arg0) == INTEGER_CST && TREE_CODE (arg1) == INTEGER_CST); The comment says specific condition needs to be satisfied if sizetype precision doesn't match that of pointers, while the code is checking "==" of precisions? Any explanation? Thanks, bin
Re: Confused about code/comment in tree.c:build2
On Fri, 31 Jan 2020, Bin.Cheng wrote: > Hi, > In tree.c:build2 there is following code/comment: > > if ((code == MINUS_EXPR || code == PLUS_EXPR || code == MULT_EXPR) > && arg0 && arg1 && tt && POINTER_TYPE_P (tt) > /* When sizetype precision doesn't match that of pointers > we need to be able to build explicit extensions or truncations > of the offset argument. */ > && TYPE_PRECISION (sizetype) == TYPE_PRECISION (tt)) > gcc_assert (TREE_CODE (arg0) == INTEGER_CST > && TREE_CODE (arg1) == INTEGER_CST); > > The comment says specific condition needs to be satisfied if sizetype > precision doesn't match that of pointers, while the code is checking > "==" of precisions? Any explanation? On targets where sizetype mode != ptr_mode the assert fires for reasons I do not remember. The comment is a bit confusing since we're dealing with plus/minus/mult here, not truncations or extensions also the assert will still allow to-be-constant-folded values (guess fold-const.c doesn't honor the restrictions). In reality minus/plus/mult should never be used on pointers. Richard.
