Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

runtests: --no-debuginfod now disables DEBUGINFOD_URLS #9950

Closed
wants to merge 1 commit into from

Conversation

bagder
Copy link
Member

@bagder bagder commented Nov 19, 2022

If this option is not used, valgrind may use this environment variable (if set) to do HTTP API request calls to get information.

@markg85
Copy link

markg85 commented Nov 19, 2022

+1 to this! Thank you for fixing it!

@jay
Copy link
Member

jay commented Nov 19, 2022

This commit needs further explanation and references, such as

This fixes ??? (I still don't understand why it is being restored)

Prior to this change DEBUGINFOD_URLS was disabled always since it was slowing down tests.

Ref: #8805

@bagder
Copy link
Member Author

bagder commented Nov 19, 2022

It is being "restored" because it unconditionally disables a valgrind feature. If you do not want to use this feature, then why have this environment variable set in the first place? As was shown, this feature is usable to some. I think that's motivation enough.

@markg85
Copy link

markg85 commented Nov 19, 2022

This fixes ???

Without repeating many posts in #8805 ...

Some distributions - like ArchLinux - don't distribute debug packages.
Instead they offer it though "debuginfod", a daemon to fetch debug symbols from a central service.

Without debug symbols valgrind errors (see #8805, the message has been posted there a couple times).
And that error causes nearly all curl tests to fail (again, only with valgrind installed).
With debug symbols (and in arch' case that means with DEBUGINFOD_URLS) these tests work just fine.

@jay
Copy link
Member

jay commented Nov 19, 2022

As was shown, this feature is usable to some. I think that's motivation enough.

I wasn't for or against it. It does need further explanation, such as...

Without debug symbols valgrind errors (see #8805, the message has been posted there a couple times).
And that error causes nearly all curl tests to fail (again, only with valgrind installed).
With debug symbols (and in arch' case that means with DEBUGINFOD_URLS) these tests work just fine.

Ok then may I suggest:

Prior to this change DEBUGINFOD_URLS was always disabled due to a report of it slowing down tests. However some distros need it to fetch debug symbols, and if it is disabled on those systems then curl tests with valgrind installed will fail.

Ref: #8805

etc

@monnerat
Copy link
Contributor

monnerat commented Nov 19, 2022

Without debug symbols valgrind errors

IMO this is a distro-specific problem: valgrind natively works very well without symbols. Otherwise how would you use it on a foreign stripped program ?
Each stock dtstro brings its own lot of problems: Arch forces you to use debuginfod, while Fedora works without but presets DEBUGINFOD_URLS at each login.

A solution for those who want to always disable it is to add a export DEBUGINFOD_URLS= line in ~/.profile.

In any case, if enabled and the full debug info for libc, ssl lib, ldap lib, etc is downloaded on a slow connection, be prepared to have timeouts at first test... And you'll have plenty of time for some beerscoffees !

tests/runtests.1 Outdated
@@ -95,6 +95,9 @@ using curl's regression test suite.
Lists all test case names.
.IP "-n"
Disable the check for and use of valgrind.
.IP "--no-debuginfod"
Delete the DEBUGINFOD_URLS variable if that is defined. Makes valgrind not
able to use this functionality.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also affects gdb when running runtests.pl -g

@markg85
Copy link

markg85 commented Nov 20, 2022

@jay

Ok then may I suggest:

Prior to this change DEBUGINFOD_URLS was always disabled due to a report of it slowing down tests. However some distros need it to fetch debug symbols, and if it is disabled on those systems then curl tests with valgrind installed will fail.

I don't see a suggestion there except stating how it works prior to this patch.
Or is your suggestion to keep it as it was? (thus broken on Arch)

@jay
Copy link
Member

jay commented Nov 20, 2022

I don't see a suggestion there except stating how it works prior to this patch.

Yes the suggestion is for @bagder's commit message.

@monnerat
Copy link
Contributor

monnerat commented Nov 20, 2022

I tried ./runtests.pl 100 on Arch linux: it fails leaving log/valgrind100:

valgrind:  Fatal error at startup: a function redirection
valgrind:  which is mandatory for this platform-tool combination
valgrind:  cannot be set up.  Details of the redirection are:
valgrind:  
valgrind:  A must-be-redirected function
valgrind:  whose name matches the pattern:      strlen
valgrind:  in an object with soname matching:   ld-linux-x86-64.so.2
valgrind:  was not found whilst processing
valgrind:  symbols from the object with soname: ld-linux-x86-64.so.2
valgrind:  
valgrind:  Possible fixes: (1, short term): install glibc's debuginfo
valgrind:  package on this machine.  (2, longer term): ask the packagers
valgrind:  for your Linux distribution to please in future ship a non-
valgrind:  stripped ld.so (or whatever the dynamic linker .so is called)
valgrind:  that exports the above-named function using the standard
valgrind:  calling conventions for this platform.  The package you need
valgrind:  to install for fix (1) is called
valgrind:  
valgrind:    On Debian, Ubuntu:                 libc6-dbg
valgrind:    On SuSE, openSuSE, Fedora, RHEL:   glibc-debuginfo
valgrind:  
valgrind:  Note that if you are debugging a 32 bit process on a
valgrind:  64 bit system, you will need a corresponding 32 bit debuginfo
valgrind:  package (e.g. libc6-dbg:i386).
valgrind:  
valgrind:  Cannot continue -- exiting now.  Sorry.

IMO this is an arch+valgrind specific problem. It should not happen. The comment says the debuginfod solution is a short term one, but something else should be done for the long term. Maybe a bug report to arch linux maintainers would be appropriate ?

In addition, it reports searching strlen in ld-linux, which sounds a bit odd unless the dynamic linker holds the other libraries' symbol tables. I checked libc.so.6 and it does export strlen.

I also tried to run DEBUGINFOD_URLS= valgrind /bin/bash from a console and it gives me the same message.

@bagder
Copy link
Member Author

bagder commented Nov 21, 2022

Are there (many) users who are likely to have DEBUGINFOD_URLS set and thus will suffer performance loss if we do this change? If so, why is that variable set if they don't want it used?

I don't understand why Arch wants this weird and special behavior, but I prefer not to make a judgement.

@monnerat
Copy link
Contributor

I don't understand why Arch wants this weird and special behavior

I'm not used to this distro, but I think they don't "want" it and it's just a bug.
Package valgrind can be installed without dependency to debuginfod, but does not work without the latter.

@markg85
Copy link

markg85 commented Nov 21, 2022

If so, why is that variable set if they don't want it used?

That variable is not set by default.
https://wiki.archlinux.org/title/Debuginfod

You have to explicitly install debuginfod to get that environment variable.

I don't understand why Arch wants this weird and special behavior, but I prefer not to make a judgement.

Just a general thing on debuginfod as i'm diving into that to figure out it's benefits. I stumbled upon this blog post:
https://developers.redhat.com/blog/2019/10/14/introducing-debuginfod-the-elfutils-debuginfo-server

From which i want to highlight this part specifically:

For example, your distro might package debuginfo and source files separately from the executable you're trying to debug and you may lack the permissions to install these packages. Or, perhaps you're debugging within a container that was not built with these resources, or maybe you simply don't want these files taking up space on your machine.

Debuginfo files are notorious for taking up large amounts of space, and it is not unusual for their size to be five to fifteen times that of the corresponding executable. debuginfod aims to resolve these problems.

It looks like Arch (and the distros using debuginfod) are ahead of the curve here causing some growing pains. The problems debuginfod tries to solve (according to the above quote) are great to have solved.

If performance is an issue then i'd suggest reaching out to the developers of debuginfod and voice your concerns there. They are probably happy to hear real world usecases.

@jzakrzewski
Copy link
Contributor

I don't think the debuginfod is slow. But downloading potentially hundreds of megabytes of debug symbols may be a problem on slow connections. I'd rather install the required symbols given a choice.
That's not to say that the debuginfod is not useful. It solves some problems while creating others and the trick is to find a golden middle.

Prior to this change DEBUGINFOD_URLS was always disabled due to a report
of it slowing down tests. However, some setups need it to fetch debug
symbols, and if it is disabled on those systems then curl tests with
valgrind will fail.

Reported-by: Mark Gaiser

Ref: #8805
Closes #9950
@bagder
Copy link
Member Author

bagder commented Nov 22, 2022

Thanks. All feedback combined, I believe this is the right PR to merge. I've amended the commit message with @jay's feedback.

@monnerat
Copy link
Contributor

For those who are interested in the Arch Linux problem:

@bagder bagder closed this in 280cbee Nov 25, 2022
@bagder bagder deleted the bagder/valgrind-debuginfod branch November 25, 2022 08:36
@bagder
Copy link
Member Author

bagder commented Nov 25, 2022

Let's monitor how this PR fares and see if we need to take further actions down the road...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants