public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
From: Simon Marchi <simon.marchi@polymtl.ca>
To: Lancelot SIX <lsix@lancelotsix.com>
Cc: gdb-patches@sourceware.org
Subject: Re: [PATCH v3] [PR gdb/27614] gdb-add-index fails on symlinks.
Date: Sun, 28 Mar 2021 13:51:02 -0400	[thread overview]
Message-ID: <2cba4c43-f085-ace3-d3f2-5f30bff426f3@polymtl.ca> (raw)
In-Reply-To: <YGC1P20n0c+uqZvb@Plymouth>



On 2021-03-28 12:56 p.m., Lancelot SIX wrote:
> Le Sun, Mar 28, 2021 at 12:11:22PM -0400, Simon Marchi a écrit :
>>
>>
>> On 2021-03-27 1:27 p.m., Lancelot SIX via Gdb-patches wrote:
>>> Since V2:
>>>         - Use GDB to follow symlink instead of readlink. Unlike
>>>           readlink, GDB is guaranteed to be available.
>>>
>>> Since V1:
>>>         - Replace '&>/dev/null' with '>/dev/null 2>&1'
>>>
>>> --
>>>
>>> PR 27614 shows that gdb-add-index fails to generate the index when its
>>> argument is a symlink.
>>>
>>> The following one liner illustrates the reported problem:
>>>
>>>         $ echo 'int main(){}'|gcc -g -x c -;ln -s a.out symlink;gdb-add-index symlink
>>>         gdb-add-index: No index was created for symlink
>>>         gdb-add-index: [Was there no debuginfo? Was there already an index?]
>>>         $ ls -l
>>>         -rwxr-xr-x 1 25712 Mar 19 23:05 a.out*
>>>         -rw------- 1  8277 Mar 19 23:05 a.out.gdb-index
>>>         lrwxrwxrwx 1     5 Mar 19 23:05 symlink -> a.out*
>>>
>>> GDB generates the .gdb-index file with a name that matches the name of
>>> the actual program (a.out.gdb-index here), not the symlink that
>>> references it.  The remaining of the script is looking for a file named
>>> after the provided argument (would be 'symlink.gdb-index' in our
>>> example).
>>>
>>> The common option to solve such issue would be to use readlink to follow
>>> the symlink.  Unfortunately, this command is not available in the POSIX
>>> standard.  This commit therefore proposes to use GDB itself to identify
>>> where the symlink points to.  This requires some parsing of GDB output.
>>> The added test should be enough to detect regression if GDB where to
>>> change the way it formats its output.
>>
>> I preferred your previous approach, compared to relying on a maintenance
>> command.  Relying on a maintenance command is fine in tests, for example,
>> but here somebody could use gdb-add-index from a given GDB version with
>> a GDB of a different version.
>>
>>   GDB=/my/newer/gdb gdb-add-index a.out
> 
> Yes, agreed. I was reluctant to rely on maint command in the first
> place, I should have kept it that way!
> 
>>
>> In the previous review, you said:
>>
>>>> Would it work with just `readlink <file>`?
>>
>>> This would fail if $file is a symlink to a symlink.  This is what
>>> ldconfig usually does (libfoo.so -> libfoo.so.x -> libfoo.so.x.y).
>>
>> Can't you just call it in a loop then?
>>
>>   while file is a symlink:
>>     file=readlink $file
>>
> 
> Yes. It would look somethink like:
> 
> 
> 	if test -L "$1"; then
> 		if command -v readlink >/dev/null 2>&1; then
> 			file="$1"
> 			while test -L "$file"; do
> 				target=$(readlink "$file")
> 				case "$target" in
> 					/*)
> 						file="$target"
> 						;;
> 					*)
> 						file="$(dirname "$file")/$target"
> 						;;
> 				esac
> 			done
> 		else
> 			echo "$myname: 'readlink' missing.  Failed to follow symlink $1." 1>&2
> 			exit 1
> 		fi
> 	else
> 		file="$1"
> 	fi

Looks good, one minor tweak I suggest is having:

	file=$1

above that and using $file after that, so we refer to $1 only once (it's
less obvious what $1 is than $file).

And maybe exit early if readlink isn't available to avoid one indent:

	if ! command -v readlink >/dev/null 2>&1; then
		echo ...
		exit 1
	fi

	... continue with using readlink ...

Simon

      reply	other threads:[~2021-03-28 17:52 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-27 17:27 Lancelot SIX
2021-03-28 16:11 ` Simon Marchi
2021-03-28 16:56   ` Lancelot SIX
2021-03-28 17:51     ` Simon Marchi [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=2cba4c43-f085-ace3-d3f2-5f30bff426f3@polymtl.ca \
    --to=simon.marchi@polymtl.ca \
    --cc=gdb-patches@sourceware.org \
    --cc=lsix@lancelotsix.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).