[PATCH] scripts: Make check-symfile.py work on alpha

Andrea Bolognani posted 1 patch 1 month ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20240120223010.308145-1-abologna@redhat.com
scripts/check-symfile.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[PATCH] scripts: Make check-symfile.py work on alpha
Posted by Andrea Bolognani 1 month ago
The script expects each of the symbols that it looks for to
be in one of three sections, which in nm(1) are described as
follows:

  T - The symbol is in the text (code) section.
  B - The symbol is in the BSS data section. This section
      typically contains zero-initialized or uninitialized
      data, although the exact behavior is system dependent.
  D - The symbol is in the initialized data section.

When building on alpha, however, some of the symbols show up
in one of two additional sections, specifically:

  S - The symbol is in an uninitialized or zero-initialized
      data section for small objects.
  G - The symbol is in an initialized data section for small
      objects.

In other words, S is the same as B and G is the same as D,
except with some optimization for small objects that for some
reason is applied on alpha but not on other architectures.

I have confirmed that, for all the symbols that the script
complained about being missing on alpha, the section is the
expected one, that is, symbols that are reported as B on x86
are reported as S on alpha, and symbols that are reported as
D on x86 are reported as G on alpha.

Note that, while the B section doesn't seem to be used at all
on alpha, at least in our case, the D section still is.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
 scripts/check-symfile.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/check-symfile.py b/scripts/check-symfile.py
index 0f6e780df0..c2ee405118 100755
--- a/scripts/check-symfile.py
+++ b/scripts/check-symfile.py
@@ -61,7 +61,7 @@ for elflib in elflibs:
 
     for line in nm:
         line = line.decode("utf-8")
-        symmatch = re.search(r'''^\S+\s(?:[TBD])\s(\S+)\s*$''', line)
+        symmatch = re.search(r'''^\S+\s(?:[TBSDG])\s(\S+)\s*$''', line)
         if symmatch is None:
             continue
 
-- 
2.43.0
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: [PATCH] scripts: Make check-symfile.py work on alpha
Posted by Daniel P. Berrangé 1 month ago
On Sat, Jan 20, 2024 at 11:30:10PM +0100, Andrea Bolognani wrote:
> The script expects each of the symbols that it looks for to
> be in one of three sections, which in nm(1) are described as
> follows:
> 
>   T - The symbol is in the text (code) section.
>   B - The symbol is in the BSS data section. This section
>       typically contains zero-initialized or uninitialized
>       data, although the exact behavior is system dependent.
>   D - The symbol is in the initialized data section.
> 
> When building on alpha, however, some of the symbols show up
> in one of two additional sections, specifically:

Again alpha is irrelevant as an architecture, so this
patch is not needed.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: Re: [PATCH] scripts: Make check-symfile.py work on alpha
Posted by Andrea Bolognani 1 month ago
On Sun, Jan 21, 2024 at 07:50:02PM +0000, Daniel P. Berrangé wrote:
> On Sat, Jan 20, 2024 at 11:30:10PM +0100, Andrea Bolognani wrote:
> > The script expects each of the symbols that it looks for to
> > be in one of three sections, which in nm(1) are described as
> > follows:
> >
> >   T - The symbol is in the text (code) section.
> >   B - The symbol is in the BSS data section. This section
> >       typically contains zero-initialized or uninitialized
> >       data, although the exact behavior is system dependent.
> >   D - The symbol is in the initialized data section.
> >
> > When building on alpha, however, some of the symbols show up
> > in one of two additional sections, specifically:
>
> Again alpha is irrelevant as an architecture, so this
> patch is not needed.

Debian builds packages on alpha and, even though it's no longer
considered a release architecture, it works just fine and keeps up
with updated software components:

  $ uname -a
  Linux debian-alpha 6.6.8-alpha-generic #1 Debian 6.6.8-1
(2023-12-22) alpha GNU/Linux

That's from a Debian installation running under qemu-system-alpha
that I've created over the weekend. Bit slow, as one would expect
when TCG is involved, but other than that perfectly functional.

In fact, this tiny patch is all that's needed to get the Debian
package for libvirt to build successfully. And it works too, at least
when it comes to the client part:

  $ ip a show dev enp0s2
  2: enp0s2: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc fq_codel
state UP group default qlen 1000
      link/ether 52:54:00:fd:42:9b brd ff:ff:ff:ff:ff:ff
      inet 192.168.124.126/24 brd 192.168.124.255 scope global dynamic enp0s2
         valid_lft 3379sec preferred_lft 3379sec
      inet6 fe80::5054:ff:fefd:429b/64 scope link proto kernel_ll
         valid_lft forever preferred_lft forever

  $ virsh -c qemu+ssh://192.168.124.1/system list
   Id   Name           State
  ------------------------------
   1    debian-alpha   running

  $ virsh -c qemu+ssh://192.168.124.1/system dumpxml debian-alpha
--xpath /domain/devices/interface/mac
  <mac address="52:54:00:fd:42:9b"/>

Here I'm connecting to the host's libvirtd from within the guest,
just for convenience. I could just as easily connect to a remote
hypervisor.

I could of course keep this patch as a downstream-only change in the
Debian package, but I see no point in not accepting it upstream,
especially since the maintenance burden it causes is literally zero.

Please reconsider :)

-- 
Andrea Bolognani / Red Hat / Virtualization
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: Re: Re: [PATCH] scripts: Make check-symfile.py work on alpha
Posted by Andrea Bolognani 4 weeks, 1 day ago
On Mon, Jan 22, 2024 at 01:24:41AM -0800, Andrea Bolognani wrote:
> On Sun, Jan 21, 2024 at 07:50:02PM +0000, Daniel P. Berrangé wrote:
> > On Sat, Jan 20, 2024 at 11:30:10PM +0100, Andrea Bolognani wrote:
> > > The script expects each of the symbols that it looks for to
> > > be in one of three sections, which in nm(1) are described as
> > > follows:
> > >
> > >   T - The symbol is in the text (code) section.
> > >   B - The symbol is in the BSS data section. This section
> > >       typically contains zero-initialized or uninitialized
> > >       data, although the exact behavior is system dependent.
> > >   D - The symbol is in the initialized data section.
> > >
> > > When building on alpha, however, some of the symbols show up
> > > in one of two additional sections, specifically:
> >
> > Again alpha is irrelevant as an architecture, so this
> > patch is not needed.
>
> Debian builds packages on alpha and, even though it's no longer
> considered a release architecture, it works just fine and keeps up
> with updated software components:
>
>   $ uname -a
>   Linux debian-alpha 6.6.8-alpha-generic #1 Debian 6.6.8-1 (2023-12-22) alpha GNU/Linux
>
> That's from a Debian installation running under qemu-system-alpha
> that I've created over the weekend. Bit slow, as one would expect
> when TCG is involved, but other than that perfectly functional.
>
> In fact, this tiny patch is all that's needed to get the Debian
> package for libvirt to build successfully. And it works too, at least
> when it comes to the client part:
>
>   $ ip a show dev enp0s2
>   2: enp0s2: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc fq_codel state UP group default qlen 1000
>       link/ether 52:54:00:fd:42:9b brd ff:ff:ff:ff:ff:ff
>       inet 192.168.124.126/24 brd 192.168.124.255 scope global dynamic enp0s2
>          valid_lft 3379sec preferred_lft 3379sec
>       inet6 fe80::5054:ff:fefd:429b/64 scope link proto kernel_ll
>          valid_lft forever preferred_lft forever
>
>   $ virsh -c qemu+ssh://192.168.124.1/system list
>    Id   Name           State
>   ------------------------------
>    1    debian-alpha   running
>
>   $ virsh -c qemu+ssh://192.168.124.1/system dumpxml debian-alpha --xpath /domain/devices/interface/mac
>   <mac address="52:54:00:fd:42:9b"/>
>
> Here I'm connecting to the host's libvirtd from within the guest,
> just for convenience. I could just as easily connect to a remote
> hypervisor.
>
> I could of course keep this patch as a downstream-only change in the
> Debian package, but I see no point in not accepting it upstream,
> especially since the maintenance burden it causes is literally zero.
>
> Please reconsider :)

Can you please take another look at this patch (as well as the other
one that contains alpha-specific changes) in light of the arguments
that I've provided above? Thanks.

-- 
Andrea Bolognani / Red Hat / Virtualization
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: [PATCH] scripts: Make check-symfile.py work on alpha
Posted by Michal Prívozník 3 weeks, 4 days ago
On 1/26/24 11:21, Andrea Bolognani wrote:
> On Mon, Jan 22, 2024 at 01:24:41AM -0800, Andrea Bolognani wrote:

>> Please reconsider :)
> 
> Can you please take another look at this patch (as well as the other
> one that contains alpha-specific changes) in light of the arguments
> that I've provided above? Thanks.
> 

I don't want to override Dan, but to me this makes sense. Unless he
disagrees, you have my:

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: [PATCH] scripts: Make check-symfile.py work on alpha
Posted by Daniel P. Berrangé 3 weeks, 4 days ago
On Tue, Jan 30, 2024 at 12:39:29PM +0100, Michal Prívozník wrote:
> On 1/26/24 11:21, Andrea Bolognani wrote:
> > On Mon, Jan 22, 2024 at 01:24:41AM -0800, Andrea Bolognani wrote:
> 
> >> Please reconsider :)
> > 
> > Can you please take another look at this patch (as well as the other
> > one that contains alpha-specific changes) in light of the arguments
> > that I've provided above? Thanks.
> > 
> 
> I don't want to override Dan, but to me this makes sense. Unless he
> disagrees, you have my:

Go for it.

I still maintain no one should waste a second of effort on an
architecture that was dead before libvirt even started as a
project, but won't block this.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: Re: [PATCH] scripts: Make check-symfile.py work on alpha
Posted by Andrea Bolognani 3 weeks, 4 days ago
On Tue, Jan 30, 2024 at 12:18:43PM +0000, Daniel P. Berrangé wrote:
> On Tue, Jan 30, 2024 at 12:39:29PM +0100, Michal Prívozník wrote:
> > On 1/26/24 11:21, Andrea Bolognani wrote:
> > > Can you please take another look at this patch (as well as the other
> > > one that contains alpha-specific changes) in light of the arguments
> > > that I've provided above? Thanks.
> >
> > I don't want to override Dan, but to me this makes sense. Unless he
> > disagrees, you have my:
>
> Go for it.
>
> I still maintain no one should waste a second of effort on an
> architecture that was dead before libvirt even started as a
> project, but won't block this.

Thanks, I appreciate that!

Any chance either of you could look at the other patch[1] as well?
The connection to alpha is sort of tangential in that case, what's
more important is that we're currently not using -fstack-protector
on aarch64 even though we should.


[1] https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/R5C57PJM3JPTXAK5Q25DS6J3CCEUTLVA/
-- 
Andrea Bolognani / Red Hat / Virtualization
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: Re: [PATCH] scripts: Make check-symfile.py work on alpha
Posted by Peter Krempa 3 weeks, 4 days ago
On Tue, Jan 30, 2024 at 07:42:10 -0800, Andrea Bolognani wrote:
> On Tue, Jan 30, 2024 at 12:18:43PM +0000, Daniel P. Berrangé wrote:
> > On Tue, Jan 30, 2024 at 12:39:29PM +0100, Michal Prívozník wrote:
> > > On 1/26/24 11:21, Andrea Bolognani wrote:
> > > > Can you please take another look at this patch (as well as the other
> > > > one that contains alpha-specific changes) in light of the arguments
> > > > that I've provided above? Thanks.
> > >
> > > I don't want to override Dan, but to me this makes sense. Unless he
> > > disagrees, you have my:
> >
> > Go for it.
> >
> > I still maintain no one should waste a second of effort on an
> > architecture that was dead before libvirt even started as a
> > project, but won't block this.
> 
> Thanks, I appreciate that!
> 
> Any chance either of you could look at the other patch[1] as well?
> The connection to alpha is sort of tangential in that case, what's
> more important is that we're currently not using -fstack-protector
> on aarch64 even though we should.

IMO there you should post a version which just enables
-fstack-protector everywhere, as in ... drop the alpha hack. That one is
starting to go beyond the 'trivial' change.
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: Re: Re: [PATCH] scripts: Make check-symfile.py work on alpha
Posted by Andrea Bolognani 3 weeks, 4 days ago
On Tue, Jan 30, 2024 at 05:36:03PM +0100, Peter Krempa wrote:
> On Tue, Jan 30, 2024 at 07:42:10 -0800, Andrea Bolognani wrote:
> > Any chance either of you could look at the other patch[1] as well?
> > The connection to alpha is sort of tangential in that case, what's
> > more important is that we're currently not using -fstack-protector
> > on aarch64 even though we should.
>
> IMO there you should post a version which just enables
> -fstack-protector everywhere, as in ... drop the alpha hack. That one is
> starting to go beyond the 'trivial' change.

That's not my preference, clearly, but I'm okay doing that.

While this patch is strictly necessary to make libvirt build
successfully on alpha, the alpha-related part of the other one only
removes a number of annoying, but ultimately harmless warnings.

One last chance for someone to ACK that patch as-is. If not, I'll
cave in and post an updated, alpha-free version :)

-- 
Andrea Bolognani / Red Hat / Virtualization
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: [PATCH] scripts: Make check-symfile.py work on alpha
Posted by Michal Prívozník 3 weeks, 4 days ago
On 1/30/24 18:04, Andrea Bolognani wrote:
> On Tue, Jan 30, 2024 at 05:36:03PM +0100, Peter Krempa wrote:
>> On Tue, Jan 30, 2024 at 07:42:10 -0800, Andrea Bolognani wrote:
>>> Any chance either of you could look at the other patch[1] as well?
>>> The connection to alpha is sort of tangential in that case, what's
>>> more important is that we're currently not using -fstack-protector
>>> on aarch64 even though we should.
>>
>> IMO there you should post a version which just enables
>> -fstack-protector everywhere, as in ... drop the alpha hack. That one is
>> starting to go beyond the 'trivial' change.
> 
> That's not my preference, clearly, but I'm okay doing that.
> 
> While this patch is strictly necessary to make libvirt build
> successfully on alpha, the alpha-related part of the other one only
> removes a number of annoying, but ultimately harmless warnings.
> 
> One last chance for someone to ACK that patch as-is. If not, I'll
> cave in and post an updated, alpha-free version :)
> 

While one could argue that this check-symfile.py patch is: a) trivial,
b) useful for other arches than just alpha (possibly, in case linker
decides to put memory into different places like S or G), it's hard to
use the same argument for -fstack-protector IMO.

Michal
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org
Re: Re: [PATCH] scripts: Make check-symfile.py work on alpha
Posted by Andrea Bolognani 3 weeks, 4 days ago
On Tue, Jan 30, 2024 at 06:31:11PM +0100, Michal Prívozník wrote:
> On 1/30/24 18:04, Andrea Bolognani wrote:
> > On Tue, Jan 30, 2024 at 05:36:03PM +0100, Peter Krempa wrote:
> >> On Tue, Jan 30, 2024 at 07:42:10 -0800, Andrea Bolognani wrote:
> >>> Any chance either of you could look at the other patch[1] as well?
> >>> The connection to alpha is sort of tangential in that case, what's
> >>> more important is that we're currently not using -fstack-protector
> >>> on aarch64 even though we should.
> >>
> >> IMO there you should post a version which just enables
> >> -fstack-protector everywhere, as in ... drop the alpha hack. That one is
> >> starting to go beyond the 'trivial' change.
> >
> > That's not my preference, clearly, but I'm okay doing that.
> >
> > While this patch is strictly necessary to make libvirt build
> > successfully on alpha, the alpha-related part of the other one only
> > removes a number of annoying, but ultimately harmless warnings.
> >
> > One last chance for someone to ACK that patch as-is. If not, I'll
> > cave in and post an updated, alpha-free version :)
>
> While one could argue that this check-symfile.py patch is: a) trivial,
> b) useful for other arches than just alpha (possibly, in case linker
> decides to put memory into different places like S or G), it's hard to
> use the same argument for -fstack-protector IMO.

Heard. v2 posted.

-- 
Andrea Bolognani / Red Hat / Virtualization
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org