public inbox for dwz@sourceware.org
 help / color / mirror / Atom feed
* [committed] Make main more readable
@ 2021-03-17 13:14 Tom de Vries
  2021-03-17 13:18 ` Jakub Jelinek
  0 siblings, 1 reply; 7+ messages in thread
From: Tom de Vries @ 2021-03-17 13:14 UTC (permalink / raw)
  To: dwz, jakub, mark

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");
 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [committed] Make main more readable
  2021-03-17 13:14 [committed] Make main more readable Tom de Vries
@ 2021-03-17 13:18 ` Jakub Jelinek
  2021-03-17 13:37   ` Tom de Vries
  0 siblings, 1 reply; 7+ messages in thread
From: Jakub Jelinek @ 2021-03-17 13:18 UTC (permalink / raw)
  To: Tom de Vries; +Cc: dwz, mark

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [committed] Make main more readable
  2021-03-17 13:18 ` Jakub Jelinek
@ 2021-03-17 13:37   ` Tom de Vries
  2021-03-17 17:28     ` Jakub Jelinek
  0 siblings, 1 reply; 7+ messages in thread
From: Tom de Vries @ 2021-03-17 13:37 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: dwz, mark

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [committed] Make main more readable
  2021-03-17 13:37   ` Tom de Vries
@ 2021-03-17 17:28     ` Jakub Jelinek
  2021-03-18 10:30       ` Tom de Vries
  0 siblings, 1 reply; 7+ messages in thread
From: Jakub Jelinek @ 2021-03-17 17:28 UTC (permalink / raw)
  To: Tom de Vries; +Cc: dwz, mark

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [committed] Make main more readable
  2021-03-17 17:28     ` Jakub Jelinek
@ 2021-03-18 10:30       ` Tom de Vries
  2021-03-18 18:52         ` Florian Weimer
  0 siblings, 1 reply; 7+ messages in thread
From: Tom de Vries @ 2021-03-18 10:30 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: dwz, mark

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [committed] Make main more readable
  2021-03-18 10:30       ` Tom de Vries
@ 2021-03-18 18:52         ` Florian Weimer
  2021-03-19 16:43           ` [committed] Change files var in main to char ** Tom de Vries
  0 siblings, 1 reply; 7+ messages in thread
From: Florian Weimer @ 2021-03-18 18:52 UTC (permalink / raw)
  To: Tom de Vries; +Cc: Jakub Jelinek, mark, dwz

* 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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [committed] Change files var in main to char **
  2021-03-18 18:52         ` Florian Weimer
@ 2021-03-19 16:43           ` Tom de Vries
  0 siblings, 0 replies; 7+ messages in thread
From: Tom de Vries @ 2021-03-19 16:43 UTC (permalink / raw)
  To: Florian Weimer; +Cc: Jakub Jelinek, mark, dwz

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-03-19 16:43 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-17 13:14 [committed] Make main more readable 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
2021-03-18 18:52         ` Florian Weimer
2021-03-19 16:43           ` [committed] Change files var in main to char ** Tom de Vries

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