[committed] Make main more readable

[committed] Make main more readable

[Tom de Vries]

Hi,

Use variables nr_files and files in main instead of optind, where possible.

Committed to trunk.

Thanks,
- Tom

Make main more readable

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

	* dwz.c (main): Use nr_files and files instead of optind.

---
 dwz.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/dwz.c b/dwz.c
index 8f64a34..c5a27be 100644
--- a/dwz.c
+++ b/dwz.c
@@ -17041,6 +17041,8 @@ main (int argc, char *argv[])
   int ret;
   const char *outfile;
   bool hardlink;
+  int nr_files;
+  const char **files;
 
   if (elf_version (EV_CURRENT) == EV_NONE)
     error (1, 0, "library out of date");
@@ -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];
 
       if (multifile != NULL)
 	{
@@ -17063,9 +17067,6 @@ main (int argc, char *argv[])
     }
   else
     {
-      int nr_files = argc - optind;
-      const char **files = (const char **)&argv[optind];
-
       if (outfile != NULL)
 	error (1, 0, "-o option not allowed for multiple files");
 

Re: [committed] Make main more readable

[Jakub Jelinek]

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?

	Jakub

Re: [committed] Make main more readable

[Tom de Vries]

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?

FWIW, I've compiled with:
...
$ make clean; make CC="gcc -Wall -fstrict-aliasing
-Wstrict-aliasing={1,2,3}"
...
and nothing in main was flagged.

OTOH, we do have things like:
...
dwz.c: In function ‘die_cu’:
dwz.c:1195:3: warning: dereferencing type-punned pointer might break
strict-aliasing rules [-Wstrict-aliasing]
   return (dw_cu_ref) die->die_parent;
   ^~~~~~
...

Thanks,
- Tom

Re: [committed] Make main more readable

[Jakub Jelinek]

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.

	Jakub

Re: [committed] Make main more readable

[Tom de Vries]

[-- 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)
     {

Re: [committed] Make main more readable

[Florian Weimer]

* Tom de Vries:

> 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. ]

I think the issue is about the char * array elements, not the individual
bytes in the strings themselves.

Thanks,
Florian

[committed] Change files var in main to char **

[Tom de Vries]

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

[ was: Re: [committed] Make main more readable ]

On 3/18/21 7:52 PM, Florian Weimer wrote:
> * Tom de Vries:
> 
>> 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. ]
> 
> I think the issue is about the char * array elements, not the individual
> bytes in the strings themselves.

I see, that explains, I somehow managed to miss that.

I've updated the log message, and committed.

Thanks,
- Tom


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

Change files var in main to char **

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.

The array element argv[i] aliases with the array element files[i],
but the types of the elements (char * vs. const char *) are not compatible types.

Since we access the elements using both types (const char * in dwz_files/dwz,
char * in getopt_long), there's a violation.

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)
     {