public inbox for dwz@sourceware.org
 help / color / mirror / Atom feed
From: Tom de Vries <tdevries@suse.de>
To: Jakub Jelinek <jakub@redhat.com>
Cc: dwz@sourceware.org, mark@klomp.org
Subject: Re: [committed] Make main more readable
Date: Thu, 18 Mar 2021 11:30:00 +0100	[thread overview]
Message-ID: <dfc806c2-553e-f335-ad56-569cda2d5038@suse.de> (raw)
In-Reply-To: <20210317172818.GI231854@tucnak>

[-- Attachment #1: Type: text/plain, Size: 2186 bytes --]

On 3/17/21 6:28 PM, Jakub Jelinek wrote:
> On Wed, Mar 17, 2021 at 02:37:57PM +0100, Tom de Vries wrote:
>> On 3/17/21 2:18 PM, Jakub Jelinek wrote:
>>> On Wed, Mar 17, 2021 at 02:14:45PM +0100, Tom de Vries wrote:
>>>> @@ -17048,10 +17050,12 @@ main (int argc, char *argv[])
>>>>    outfile = NULL;
>>>>    hardlink = false;
>>>>    parse_args (argc, argv, &hardlink, &outfile);
>>>> +  nr_files = argc - optind;
>>>> +  files = (const char **)&argv[optind];
>>>>  
>>>> -  if (optind == argc || optind + 1 == argc)
>>>> +  if (nr_files <= 1)
>>>>      {
>>>> -      const char *file = optind == argc ? "a.out" : argv[optind];
>>>> +      const char *file = nr_files == 0 ? "a.out" : files[0];
>>>
>>> Isn't that aliasing violation?
>>
>> Sorry, I don't see it, can you be specific about which entity is
>> accessed with an incompatible type?
> 
> char * and const char * are different types from C aliasing POV I think.
> Now, as these are arguments of main and argv can be const char **
> or char ** interchangeably, what is the dynamic type is a little bit fuzzy,
> but I'd say mixing accesses to argv array elements, accessing some of them
> through char * effective type (e.g. in getopt_long) and others through
> const char * effective type is problematic.
> Making files char ** and then casting if needed (say (const char *)(files[0]))
> is fine of course.

I've prepared a patch implementing that suggestion.  Does that address
your concerns?

[ FWIW, I've read through a C99 draft pdf to refresh my memory on this
topic.  Looking at the effective type of **argv, AFAIU it falls into the
catagory "For all other accesses to an object having no declared type,
the effective type of the object is simply the type of the lvalue used
for the access".  In other words, the effective type is char.

Then I read:
...
An object shall have its stored value accessed only by an lvalue
expression that has one of the following types:
— a qualified version of a type compatible with the effective type of
  the object,
...

So, const char is a qualified version of a type char compatible with the
effective type of the object (char).

So I still don't see the problem. ]

Thanks,
- Tom

[-- Attachment #2: 0001-Change-files-var-in-main-to-char.patch --]
[-- Type: text/x-patch, Size: 1835 bytes --]

Change files var in main to char **

There's a concern that the cast on &argv[optind] to const char **:
...
main (int argc, char *argv[])
{
  ...
  const char **files;
  ...
  files = (const char **)&argv[optind];
...
violates the aliasing rules.

Fix this by changing the type of files to char **.

2021-03-18  Tom de Vries  <tdevries@suse.de>

	* dwz.c (dwz): Make files param a char **.
	(dwz_files): Make files param char *[].
	(main): Make files var a char **.

---
 dwz.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/dwz.c b/dwz.c
index 7b67960..a7aa23b 100644
--- a/dwz.c
+++ b/dwz.c
@@ -15267,7 +15267,7 @@ struct file_result
    over FILE.  */
 static int
 dwz (const char *file, const char *outfile, struct file_result *res,
-     struct file_result *resa, const char **files)
+     struct file_result *resa, char **files)
 {
   DSO *dso;
   int ret = 0, fd;
@@ -16285,7 +16285,7 @@ dwz_one_file (const char *file, const char *outfile)
 /* Dwarf-compress FILES.  If HARDLINK, detect if some files are hardlinks and
    if so, dwarf-compress just one, and relink the others.  */
 static int
-dwz_files (int nr_files, const char *files[], bool hardlink)
+dwz_files (int nr_files, char *files[], bool hardlink)
 {
   int ret = 0;
   int i;
@@ -16413,7 +16413,7 @@ main (int argc, char *argv[])
   const char *outfile;
   bool hardlink;
   int nr_files;
-  const char **files;
+  char **files;
 
   if (elf_version (EV_CURRENT) == EV_NONE)
     error (1, 0, "library out of date");
@@ -16422,7 +16422,7 @@ main (int argc, char *argv[])
   hardlink = false;
   parse_args (argc, argv, &hardlink, &outfile);
   nr_files = argc - optind;
-  files = (const char **)&argv[optind];
+  files = &argv[optind];
 
   if (nr_files <= 1)
     {

  reply	other threads:[~2021-03-18 10:30 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-17 13:14 Tom de Vries
2021-03-17 13:18 ` Jakub Jelinek
2021-03-17 13:37   ` Tom de Vries
2021-03-17 17:28     ` Jakub Jelinek
2021-03-18 10:30       ` Tom de Vries [this message]
2021-03-18 18:52         ` Florian Weimer
2021-03-19 16:43           ` [committed] Change files var in main to char ** Tom de Vries

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=dfc806c2-553e-f335-ad56-569cda2d5038@suse.de \
    --to=tdevries@suse.de \
    --cc=dwz@sourceware.org \
    --cc=jakub@redhat.com \
    --cc=mark@klomp.org \
    /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).