| Message ID | 20220505142137.51306-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | build/xen: fix symbol generation with LLVM LD | expand |
On 05.05.2022 16:21, Roger Pau Monne wrote: > Current LLVM LD implementation will turn global hidden symbols in > object files into local ones when generating the .symtab of the Xen > binary image. > > This is different from GNU ld implementation, that will only do the > conversion (or remove the symbols) when generation .dynsym but not > .symtab. Such conversion breaks the processing of symbols done by > tools/symbols. > > Use protected symbol visibility instead of hidden, as that preserves > the symbol binding while not generating GOT or PLT indirections that > are not compatible with some of the inline assembly constructs > currently used. > > While there also make the visibility setting compiler support > non-optional: compilers not supporting it won't be able to build Xen > anyway, and will just throw a compiler error sooner rather than later > during the build. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com>
On 05.05.2022 16:21, Roger Pau Monne wrote: > --- a/xen/include/xen/compiler.h > +++ b/xen/include/xen/compiler.h > @@ -125,10 +125,11 @@ > #define __must_be_array(a) \ > BUILD_BUG_ON_ZERO(__builtin_types_compatible_p(typeof(a), typeof(&a[0]))) > > -#ifdef CONFIG_CC_HAS_VISIBILITY_ATTRIBUTE > -/* Results in more efficient PIC code (no indirections through GOT or PLT). */ > -#pragma GCC visibility push(hidden) > -#endif > +/* > + * Results in more efficient PIC code (no indirections through GOT or PLT) > + * and is also required by some of the assembly constructs. > + */ > +#pragma GCC visibility push(protected) > > /* Make the optimizer believe the variable can be manipulated arbitrarily. */ > #define OPTIMIZER_HIDE_VAR(var) __asm__ ( "" : "+g" (var) ) This has failed my pre-push build test, with massive amounts of errors about asm() constraints in the alternative call infrastructure. This was with gcc 11.3.0. Jan
On Fri, May 06, 2022 at 02:56:56PM +0200, Jan Beulich wrote: > On 05.05.2022 16:21, Roger Pau Monne wrote: > > --- a/xen/include/xen/compiler.h > > +++ b/xen/include/xen/compiler.h > > @@ -125,10 +125,11 @@ > > #define __must_be_array(a) \ > > BUILD_BUG_ON_ZERO(__builtin_types_compatible_p(typeof(a), typeof(&a[0]))) > > > > -#ifdef CONFIG_CC_HAS_VISIBILITY_ATTRIBUTE > > -/* Results in more efficient PIC code (no indirections through GOT or PLT). */ > > -#pragma GCC visibility push(hidden) > > -#endif > > +/* > > + * Results in more efficient PIC code (no indirections through GOT or PLT) > > + * and is also required by some of the assembly constructs. > > + */ > > +#pragma GCC visibility push(protected) > > > > /* Make the optimizer believe the variable can be manipulated arbitrarily. */ > > #define OPTIMIZER_HIDE_VAR(var) __asm__ ( "" : "+g" (var) ) > > This has failed my pre-push build test, with massive amounts of errors > about asm() constraints in the alternative call infrastructure. This > was with gcc 11.3.0. Hm, great. I guess I will have to use protected with clang and hidden with gcc then, for lack of a better solution. I'm slightly confused as to why my godbolt example: https://godbolt.org/z/chTnMWxeP Seems to work with gcc 11 then. I will have to investigate a bit I think. Thanks, and sorry for the trouble.
On Fri, May 06, 2022 at 03:31:12PM +0200, Roger Pau Monné wrote: > On Fri, May 06, 2022 at 02:56:56PM +0200, Jan Beulich wrote: > > On 05.05.2022 16:21, Roger Pau Monne wrote: > > > --- a/xen/include/xen/compiler.h > > > +++ b/xen/include/xen/compiler.h > > > @@ -125,10 +125,11 @@ > > > #define __must_be_array(a) \ > > > BUILD_BUG_ON_ZERO(__builtin_types_compatible_p(typeof(a), typeof(&a[0]))) > > > > > > -#ifdef CONFIG_CC_HAS_VISIBILITY_ATTRIBUTE > > > -/* Results in more efficient PIC code (no indirections through GOT or PLT). */ > > > -#pragma GCC visibility push(hidden) > > > -#endif > > > +/* > > > + * Results in more efficient PIC code (no indirections through GOT or PLT) > > > + * and is also required by some of the assembly constructs. > > > + */ > > > +#pragma GCC visibility push(protected) > > > > > > /* Make the optimizer believe the variable can be manipulated arbitrarily. */ > > > #define OPTIMIZER_HIDE_VAR(var) __asm__ ( "" : "+g" (var) ) > > > > This has failed my pre-push build test, with massive amounts of errors > > about asm() constraints in the alternative call infrastructure. This > > was with gcc 11.3.0. > > Hm, great. I guess I will have to use protected with clang and hidden > with gcc then, for lack of a better solution. > > I'm slightly confused as to why my godbolt example: > > https://godbolt.org/z/chTnMWxeP > > Seems to work with gcc 11 then. I will have to investigate a bit I > think. So it seems the problem is explicitly with constructs like: void (*foo)(void); void test(void) { asm volatile (".long [addr]" :: [addr] "i" (&(foo))); } See: https://godbolt.org/z/TYqeGdWsn AFAICT gcc will consider the function pointer foo to go through the GOT/PLT redirection table, while clang will not. I think gcc behavior is correct because in theory foo could be set from a different module? protect only guarantees that references to local functions cannot be overwritten, but not external ones. I don't really see a good way to fix this, rather that setting different visibilities based on the compiler. clang would use protected and gcc would use hidden. I think it's unlikely to have a toolstack setup to use gcc as the compiler and LLVM LD as the linker, which would be the problematic configuration, and even in that case it's kind of a cosmetic issue with symbol resolution, binary output from the linker would still be correct. Let me know if that seems acceptable. Thanks, Roger.
On 06.05.2022 17:35, Roger Pau Monné wrote: > On Fri, May 06, 2022 at 03:31:12PM +0200, Roger Pau Monné wrote: >> On Fri, May 06, 2022 at 02:56:56PM +0200, Jan Beulich wrote: >>> On 05.05.2022 16:21, Roger Pau Monne wrote: >>>> --- a/xen/include/xen/compiler.h >>>> +++ b/xen/include/xen/compiler.h >>>> @@ -125,10 +125,11 @@ >>>> #define __must_be_array(a) \ >>>> BUILD_BUG_ON_ZERO(__builtin_types_compatible_p(typeof(a), typeof(&a[0]))) >>>> >>>> -#ifdef CONFIG_CC_HAS_VISIBILITY_ATTRIBUTE >>>> -/* Results in more efficient PIC code (no indirections through GOT or PLT). */ >>>> -#pragma GCC visibility push(hidden) >>>> -#endif >>>> +/* >>>> + * Results in more efficient PIC code (no indirections through GOT or PLT) >>>> + * and is also required by some of the assembly constructs. >>>> + */ >>>> +#pragma GCC visibility push(protected) >>>> >>>> /* Make the optimizer believe the variable can be manipulated arbitrarily. */ >>>> #define OPTIMIZER_HIDE_VAR(var) __asm__ ( "" : "+g" (var) ) >>> >>> This has failed my pre-push build test, with massive amounts of errors >>> about asm() constraints in the alternative call infrastructure. This >>> was with gcc 11.3.0. >> >> Hm, great. I guess I will have to use protected with clang and hidden >> with gcc then, for lack of a better solution. >> >> I'm slightly confused as to why my godbolt example: >> >> https://godbolt.org/z/chTnMWxeP >> >> Seems to work with gcc 11 then. I will have to investigate a bit I >> think. > > So it seems the problem is explicitly with constructs like: > > void (*foo)(void); > > void test(void) > { > asm volatile (".long [addr]" :: [addr] "i" (&(foo))); > } > > See: > > https://godbolt.org/z/TYqeGdWsn > > AFAICT gcc will consider the function pointer foo to go through the > GOT/PLT redirection table, while clang will not. I think gcc behavior > is correct because in theory foo could be set from a different module? > protect only guarantees that references to local functions cannot be > overwritten, but not external ones. Right, since there's no way to tell the compiler that the symbol will be resolved in the same "module". > I don't really see a good way to fix this, rather that setting > different visibilities based on the compiler. clang would use > protected and gcc would use hidden. If gcc's behavior is indeed correct, then moving to protected with clang would set us up for going through GOT/PLT there - either right away (if the implement this like gcc), or once they correct their behavior. I don't think we want that. Therefore I think we want to alter visibility between compilation and linking (i.e. presumably right in prelink.o), going from compile-time hidden to link-time protected. That would likely be closer to what your original patch did (sadly there's no "convert <visibility1> to <visibility2> option to objcopy, and making it have one wouldn't really help us here; it's also not clear to me whether llvm comes with its own objcopy, or whether they re-use GNU's). > I think it's unlikely to have a > toolstack setup to use gcc as the compiler and LLVM LD as the > linker, which would be the problematic configuration, and even in that > case it's kind of a cosmetic issue with symbol resolution, binary > output from the linker would still be correct. While likely moot with the above, I agree that we could make such an assumption if need be. Jan > Let me know if that seems acceptable. > > Thanks, Roger. >
On Sun, May 08, 2022 at 10:34:43AM +0200, Jan Beulich wrote: > On 06.05.2022 17:35, Roger Pau Monné wrote: > > On Fri, May 06, 2022 at 03:31:12PM +0200, Roger Pau Monné wrote: > >> On Fri, May 06, 2022 at 02:56:56PM +0200, Jan Beulich wrote: > >>> On 05.05.2022 16:21, Roger Pau Monne wrote: > >>>> --- a/xen/include/xen/compiler.h > >>>> +++ b/xen/include/xen/compiler.h > >>>> @@ -125,10 +125,11 @@ > >>>> #define __must_be_array(a) \ > >>>> BUILD_BUG_ON_ZERO(__builtin_types_compatible_p(typeof(a), typeof(&a[0]))) > >>>> > >>>> -#ifdef CONFIG_CC_HAS_VISIBILITY_ATTRIBUTE > >>>> -/* Results in more efficient PIC code (no indirections through GOT or PLT). */ > >>>> -#pragma GCC visibility push(hidden) > >>>> -#endif > >>>> +/* > >>>> + * Results in more efficient PIC code (no indirections through GOT or PLT) > >>>> + * and is also required by some of the assembly constructs. > >>>> + */ > >>>> +#pragma GCC visibility push(protected) > >>>> > >>>> /* Make the optimizer believe the variable can be manipulated arbitrarily. */ > >>>> #define OPTIMIZER_HIDE_VAR(var) __asm__ ( "" : "+g" (var) ) > >>> > >>> This has failed my pre-push build test, with massive amounts of errors > >>> about asm() constraints in the alternative call infrastructure. This > >>> was with gcc 11.3.0. > >> > >> Hm, great. I guess I will have to use protected with clang and hidden > >> with gcc then, for lack of a better solution. > >> > >> I'm slightly confused as to why my godbolt example: > >> > >> https://godbolt.org/z/chTnMWxeP > >> > >> Seems to work with gcc 11 then. I will have to investigate a bit I > >> think. > > > > So it seems the problem is explicitly with constructs like: > > > > void (*foo)(void); > > > > void test(void) > > { > > asm volatile (".long [addr]" :: [addr] "i" (&(foo))); > > } > > > > See: > > > > https://godbolt.org/z/TYqeGdWsn > > > > AFAICT gcc will consider the function pointer foo to go through the > > GOT/PLT redirection table, while clang will not. I think gcc behavior > > is correct because in theory foo could be set from a different module? > > protect only guarantees that references to local functions cannot be > > overwritten, but not external ones. > > Right, since there's no way to tell the compiler that the symbol will > be resolved in the same "module". > > > I don't really see a good way to fix this, rather that setting > > different visibilities based on the compiler. clang would use > > protected and gcc would use hidden. > > If gcc's behavior is indeed correct, then moving to protected with > clang would set us up for going through GOT/PLT there - either right > away (if the implement this like gcc), or once they correct their > behavior. I don't think we want that. Therefore I think we want to > alter visibility between compilation and linking (i.e. presumably > right in prelink.o), going from compile-time hidden to link-time > protected. That would likely be closer to what your original patch > did (sadly there's no "convert <visibility1> to <visibility2> option > to objcopy, and making it have one wouldn't really help us here; > it's also not clear to me whether llvm comes with its own objcopy, > or whether they re-use GNU's). So I've raised the difference in protected behavior between gcc and clang: https://discourse.llvm.org/t/gcc-vs-clang-differences-in-protected-visibility-implementation It's no clear to me whether clang would switch it's implementation, but it also seems fragile to rely on global protected function pointers not going through the GOT. Do you have any recommendation as to how to change symbol visibility? I've been looking at objcopy, but I don't seem to find a way to do it. Thanks, Roger.
On 16.05.2022 10:01, Roger Pau Monné wrote: > On Sun, May 08, 2022 at 10:34:43AM +0200, Jan Beulich wrote: >> On 06.05.2022 17:35, Roger Pau Monné wrote: >>> On Fri, May 06, 2022 at 03:31:12PM +0200, Roger Pau Monné wrote: >>>> On Fri, May 06, 2022 at 02:56:56PM +0200, Jan Beulich wrote: >>>>> On 05.05.2022 16:21, Roger Pau Monne wrote: >>>>>> --- a/xen/include/xen/compiler.h >>>>>> +++ b/xen/include/xen/compiler.h >>>>>> @@ -125,10 +125,11 @@ >>>>>> #define __must_be_array(a) \ >>>>>> BUILD_BUG_ON_ZERO(__builtin_types_compatible_p(typeof(a), typeof(&a[0]))) >>>>>> >>>>>> -#ifdef CONFIG_CC_HAS_VISIBILITY_ATTRIBUTE >>>>>> -/* Results in more efficient PIC code (no indirections through GOT or PLT). */ >>>>>> -#pragma GCC visibility push(hidden) >>>>>> -#endif >>>>>> +/* >>>>>> + * Results in more efficient PIC code (no indirections through GOT or PLT) >>>>>> + * and is also required by some of the assembly constructs. >>>>>> + */ >>>>>> +#pragma GCC visibility push(protected) >>>>>> >>>>>> /* Make the optimizer believe the variable can be manipulated arbitrarily. */ >>>>>> #define OPTIMIZER_HIDE_VAR(var) __asm__ ( "" : "+g" (var) ) >>>>> >>>>> This has failed my pre-push build test, with massive amounts of errors >>>>> about asm() constraints in the alternative call infrastructure. This >>>>> was with gcc 11.3.0. >>>> >>>> Hm, great. I guess I will have to use protected with clang and hidden >>>> with gcc then, for lack of a better solution. >>>> >>>> I'm slightly confused as to why my godbolt example: >>>> >>>> https://godbolt.org/z/chTnMWxeP >>>> >>>> Seems to work with gcc 11 then. I will have to investigate a bit I >>>> think. >>> >>> So it seems the problem is explicitly with constructs like: >>> >>> void (*foo)(void); >>> >>> void test(void) >>> { >>> asm volatile (".long [addr]" :: [addr] "i" (&(foo))); >>> } >>> >>> See: >>> >>> https://godbolt.org/z/TYqeGdWsn >>> >>> AFAICT gcc will consider the function pointer foo to go through the >>> GOT/PLT redirection table, while clang will not. I think gcc behavior >>> is correct because in theory foo could be set from a different module? >>> protect only guarantees that references to local functions cannot be >>> overwritten, but not external ones. >> >> Right, since there's no way to tell the compiler that the symbol will >> be resolved in the same "module". >> >>> I don't really see a good way to fix this, rather that setting >>> different visibilities based on the compiler. clang would use >>> protected and gcc would use hidden. >> >> If gcc's behavior is indeed correct, then moving to protected with >> clang would set us up for going through GOT/PLT there - either right >> away (if the implement this like gcc), or once they correct their >> behavior. I don't think we want that. Therefore I think we want to >> alter visibility between compilation and linking (i.e. presumably >> right in prelink.o), going from compile-time hidden to link-time >> protected. That would likely be closer to what your original patch >> did (sadly there's no "convert <visibility1> to <visibility2> option >> to objcopy, and making it have one wouldn't really help us here; >> it's also not clear to me whether llvm comes with its own objcopy, >> or whether they re-use GNU's). > > So I've raised the difference in protected behavior between gcc and > clang: > > https://discourse.llvm.org/t/gcc-vs-clang-differences-in-protected-visibility-implementation > > It's no clear to me whether clang would switch it's implementation, > but it also seems fragile to rely on global protected function > pointers not going through the GOT. I agree, and I don't view it as likely that they would change their code. GNU objcopy offers a separate step for that conversion (--localize-hidden), but .. > Do you have any recommendation as to how to change symbol visibility? > I've been looking at objcopy, but I don't seem to find a way to do > it. ... kind of unexpectedly doesn't offer means to alter visibility. So I guess you/we need to turn back to your original RFC approach, no matter that it wasn't really pretty. Jan
Hi, It seems that this patch has been stale for more than 2 months and from the discussion in this thread I think some actions need to be taken from the author. So I am sending this email as a gentle reminder. Kind regards, Henry > -----Original Message----- > Subject: [PATCH] build/xen: fix symbol generation with LLVM LD > > Current LLVM LD implementation will turn global hidden symbols in > object files into local ones when generating the .symtab of the Xen > binary image. > > This is different from GNU ld implementation, that will only do the > conversion (or remove the symbols) when generation .dynsym but not > .symtab. Such conversion breaks the processing of symbols done by > tools/symbols. > > Use protected symbol visibility instead of hidden, as that preserves > the symbol binding while not generating GOT or PLT indirections that > are not compatible with some of the inline assembly constructs > currently used. > > While there also make the visibility setting compiler support > non-optional: compilers not supporting it won't be able to build Xen > anyway, and will just throw a compiler error sooner rather than later > during the build. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > xen/Kconfig | 4 ---- > xen/include/xen/compiler.h | 9 +++++---- > 2 files changed, 5 insertions(+), 8 deletions(-) > > diff --git a/xen/Kconfig b/xen/Kconfig > index 134e6e68ad..a9182fb13d 100644 > --- a/xen/Kconfig > +++ b/xen/Kconfig > @@ -29,10 +29,6 @@ config LD_IS_GNU > config LD_IS_LLVM > def_bool $(success,$(LD) --version | head -n 1 | grep -q "^LLD") > > -# -fvisibility=hidden reduces -fpic cost, if it's available > -config CC_HAS_VISIBILITY_ATTRIBUTE > - def_bool $(cc-option,-fvisibility=hidden) > - > # Use -f{function,data}-sections compiler parameters > config CC_SPLIT_SECTIONS > bool > diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h > index 933aec09a9..c144b17217 100644 > --- a/xen/include/xen/compiler.h > +++ b/xen/include/xen/compiler.h > @@ -125,10 +125,11 @@ > #define __must_be_array(a) \ > BUILD_BUG_ON_ZERO(__builtin_types_compatible_p(typeof(a), > typeof(&a[0]))) > > -#ifdef CONFIG_CC_HAS_VISIBILITY_ATTRIBUTE > -/* Results in more efficient PIC code (no indirections through GOT or PLT). */ > -#pragma GCC visibility push(hidden) > -#endif > +/* > + * Results in more efficient PIC code (no indirections through GOT or PLT) > + * and is also required by some of the assembly constructs. > + */ > +#pragma GCC visibility push(protected) > > /* Make the optimizer believe the variable can be manipulated arbitrarily. */ > #define OPTIMIZER_HIDE_VAR(var) __asm__ ( "" : "+g" (var) ) > -- > 2.36.0 >
diff --git a/xen/Kconfig b/xen/Kconfig index 134e6e68ad..a9182fb13d 100644 --- a/xen/Kconfig +++ b/xen/Kconfig @@ -29,10 +29,6 @@ config LD_IS_GNU config LD_IS_LLVM def_bool $(success,$(LD) --version | head -n 1 | grep -q "^LLD") -# -fvisibility=hidden reduces -fpic cost, if it's available -config CC_HAS_VISIBILITY_ATTRIBUTE - def_bool $(cc-option,-fvisibility=hidden) - # Use -f{function,data}-sections compiler parameters config CC_SPLIT_SECTIONS bool diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h index 933aec09a9..c144b17217 100644 --- a/xen/include/xen/compiler.h +++ b/xen/include/xen/compiler.h @@ -125,10 +125,11 @@ #define __must_be_array(a) \ BUILD_BUG_ON_ZERO(__builtin_types_compatible_p(typeof(a), typeof(&a[0]))) -#ifdef CONFIG_CC_HAS_VISIBILITY_ATTRIBUTE -/* Results in more efficient PIC code (no indirections through GOT or PLT). */ -#pragma GCC visibility push(hidden) -#endif +/* + * Results in more efficient PIC code (no indirections through GOT or PLT) + * and is also required by some of the assembly constructs. + */ +#pragma GCC visibility push(protected) /* Make the optimizer believe the variable can be manipulated arbitrarily. */ #define OPTIMIZER_HIDE_VAR(var) __asm__ ( "" : "+g" (var) )
Current LLVM LD implementation will turn global hidden symbols in object files into local ones when generating the .symtab of the Xen binary image. This is different from GNU ld implementation, that will only do the conversion (or remove the symbols) when generation .dynsym but not .symtab. Such conversion breaks the processing of symbols done by tools/symbols. Use protected symbol visibility instead of hidden, as that preserves the symbol binding while not generating GOT or PLT indirections that are not compatible with some of the inline assembly constructs currently used. While there also make the visibility setting compiler support non-optional: compilers not supporting it won't be able to build Xen anyway, and will just throw a compiler error sooner rather than later during the build. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- xen/Kconfig | 4 ---- xen/include/xen/compiler.h | 9 +++++---- 2 files changed, 5 insertions(+), 8 deletions(-)