| Message ID | 20190318012150.2951-1-james.hilliard1@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [libbacktrace] Initialize st in elf_is_symlink | expand |
On Sun, Mar 17, 2019 at 6:22 PM <james.hilliard1@gmail.com> wrote: > > From: James Hilliard <james.hilliard1@gmail.com> > > Fixes error: ‘st.st_mode’ may be used uninitialized in this function > --- > libbacktrace/elf.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libbacktrace/elf.c b/libbacktrace/elf.c > index f3988ec02a0..714bfec965d 100644 > --- a/libbacktrace/elf.c > +++ b/libbacktrace/elf.c > @@ -765,7 +765,7 @@ elf_syminfo (struct backtrace_state *state, uintptr_t addr, > static int > elf_is_symlink (const char *filename) > { > - struct stat st; > + struct stat st={0}; > > if (lstat (filename, &st) < 0) > return 0; I can't see why that is needed. The variable is initialized right there on the next non-blank line. If the compiler is giving a warning, then we need to fix the compiler. Ian
On Mon, Mar 18, 2019 at 11:19 AM Ian Lance Taylor <iant@google.com> wrote: > > On Sun, Mar 17, 2019 at 6:22 PM <james.hilliard1@gmail.com> wrote: > > > > From: James Hilliard <james.hilliard1@gmail.com> > > > > Fixes error: ‘st.st_mode’ may be used uninitialized in this function > > --- > > libbacktrace/elf.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/libbacktrace/elf.c b/libbacktrace/elf.c > > index f3988ec02a0..714bfec965d 100644 > > --- a/libbacktrace/elf.c > > +++ b/libbacktrace/elf.c > > @@ -765,7 +765,7 @@ elf_syminfo (struct backtrace_state *state, uintptr_t addr, > > static int > > elf_is_symlink (const char *filename) > > { > > - struct stat st; > > + struct stat st={0}; > > > > if (lstat (filename, &st) < 0) > > return 0; > > I can't see why that is needed. The variable is initialized right > there on the next non-blank line. If the compiler is giving a > warning, then we need to fix the compiler. This is the message I get: ../../../../libsanitizer/libbacktrace/../../libbacktrace/elf.c: In function ‘elf_is_symlink’: ../../../../libsanitizer/libbacktrace/../../libbacktrace/elf.c:772:21: error: ‘st.st_mode’ may be used uninitialized in this function [-Werror=maybe-uninitialized] return S_ISLNK (st.st_mode); ^ > > Ian
On Mon, Mar 18, 2019 at 11:57 AM James Hilliard <james.hilliard1@gmail.com> wrote: > > On Mon, Mar 18, 2019 at 11:19 AM Ian Lance Taylor <iant@google.com> wrote: > > > > On Sun, Mar 17, 2019 at 6:22 PM <james.hilliard1@gmail.com> wrote: > > > > > > From: James Hilliard <james.hilliard1@gmail.com> > > > > > > Fixes error: ‘st.st_mode’ may be used uninitialized in this function > > > --- > > > libbacktrace/elf.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/libbacktrace/elf.c b/libbacktrace/elf.c > > > index f3988ec02a0..714bfec965d 100644 > > > --- a/libbacktrace/elf.c > > > +++ b/libbacktrace/elf.c > > > @@ -765,7 +765,7 @@ elf_syminfo (struct backtrace_state *state, uintptr_t addr, > > > static int > > > elf_is_symlink (const char *filename) > > > { > > > - struct stat st; > > > + struct stat st={0}; > > > > > > if (lstat (filename, &st) < 0) > > > return 0; > > > > I can't see why that is needed. The variable is initialized right > > there on the next non-blank line. If the compiler is giving a > > warning, then we need to fix the compiler. > This is the message I get: > ../../../../libsanitizer/libbacktrace/../../libbacktrace/elf.c: In > function ‘elf_is_symlink’: > ../../../../libsanitizer/libbacktrace/../../libbacktrace/elf.c:772:21: > error: ‘st.st_mode’ may be used uninitialized in this function > [-Werror=maybe-uninitialized] > return S_ISLNK (st.st_mode); > ^ Thanks, but I'm saying that if you look at the code you can see that st is clearly initialized, by the call to lstat. I would like to see an explanation for why you are seeing that warning before changing the code to disable it. Initializing st should not be necessary here. For example, perhaps lstat is a macro when compiling libsanitizer; if that is the underlying problem, then we should fix the macro, not this code. Ian
On Mon, Mar 18, 2019 at 2:05 PM Ian Lance Taylor <iant@google.com> wrote: > > On Mon, Mar 18, 2019 at 11:57 AM James Hilliard > <james.hilliard1@gmail.com> wrote: > > > > On Mon, Mar 18, 2019 at 11:19 AM Ian Lance Taylor <iant@google.com> wrote: > > > > > > On Sun, Mar 17, 2019 at 6:22 PM <james.hilliard1@gmail.com> wrote: > > > > > > > > From: James Hilliard <james.hilliard1@gmail.com> > > > > > > > > Fixes error: ‘st.st_mode’ may be used uninitialized in this function > > > > --- > > > > libbacktrace/elf.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/libbacktrace/elf.c b/libbacktrace/elf.c > > > > index f3988ec02a0..714bfec965d 100644 > > > > --- a/libbacktrace/elf.c > > > > +++ b/libbacktrace/elf.c > > > > @@ -765,7 +765,7 @@ elf_syminfo (struct backtrace_state *state, uintptr_t addr, > > > > static int > > > > elf_is_symlink (const char *filename) > > > > { > > > > - struct stat st; > > > > + struct stat st={0}; > > > > > > > > if (lstat (filename, &st) < 0) > > > > return 0; > > > > > > I can't see why that is needed. The variable is initialized right > > > there on the next non-blank line. If the compiler is giving a > > > warning, then we need to fix the compiler. > > This is the message I get: > > ../../../../libsanitizer/libbacktrace/../../libbacktrace/elf.c: In > > function ‘elf_is_symlink’: > > ../../../../libsanitizer/libbacktrace/../../libbacktrace/elf.c:772:21: > > error: ‘st.st_mode’ may be used uninitialized in this function > > [-Werror=maybe-uninitialized] > > return S_ISLNK (st.st_mode); > > ^ > > Thanks, but I'm saying that if you look at the code you can see that > st is clearly initialized, by the call to lstat. I would like to see > an explanation for why you are seeing that warning before changing the > code to disable it. Initializing st should not be necessary here. > For example, perhaps lstat is a macro when compiling libsanitizer; if > that is the underlying problem, then we should fix the macro, not this > code. Yeah, I'm not sure why the compiler thinks lstat isn't initializing st. What should I do to debug this further? > > Ian
On Mon, Mar 18, 2019 at 04:41:05PM -0600, James Hilliard wrote: > > Thanks, but I'm saying that if you look at the code you can see that > > st is clearly initialized, by the call to lstat. I would like to see > > an explanation for why you are seeing that warning before changing the > > code to disable it. Initializing st should not be necessary here. > > For example, perhaps lstat is a macro when compiling libsanitizer; if > > that is the underlying problem, then we should fix the macro, not this > > code. > Yeah, I'm not sure why the compiler thinks lstat isn't initializing st. > What should I do to debug this further? Guess you should start by telling us which OS it is on (I can't reproduce this warning on x86_64-linux nor i686-linux with glibc 2.28), looking at preprocessed source to see what exactly lstat does (e.g. if it is some macro or inline function and what exactly it is doing). Jakub
On Mon, Mar 18, 2019 at 4:51 PM Jakub Jelinek <jakub@redhat.com> wrote: > > On Mon, Mar 18, 2019 at 04:41:05PM -0600, James Hilliard wrote: > > > Thanks, but I'm saying that if you look at the code you can see that > > > st is clearly initialized, by the call to lstat. I would like to see > > > an explanation for why you are seeing that warning before changing the > > > code to disable it. Initializing st should not be necessary here. > > > For example, perhaps lstat is a macro when compiling libsanitizer; if > > > that is the underlying problem, then we should fix the macro, not this > > > code. > > Yeah, I'm not sure why the compiler thinks lstat isn't initializing st. > > What should I do to debug this further? > > Guess you should start by telling us which OS it is on (I can't reproduce > this warning on x86_64-linux nor i686-linux with glibc 2.28), looking at > preprocessed source to see what exactly lstat does (e.g. if it is some macro > or inline function and what exactly it is doing). I am cross compiling with buildroot master branch using ubuntu 18.10. I am building gcc 8.3.0 and glibc 2.29 for the cross toolchain. The build and target systems are both x86_64. > > Jakub
On 3/18/19 5:07 PM, James Hilliard wrote: > On Mon, Mar 18, 2019 at 4:51 PM Jakub Jelinek <jakub@redhat.com> wrote: >> >> On Mon, Mar 18, 2019 at 04:41:05PM -0600, James Hilliard wrote: >>>> Thanks, but I'm saying that if you look at the code you can see that >>>> st is clearly initialized, by the call to lstat. I would like to see >>>> an explanation for why you are seeing that warning before changing the >>>> code to disable it. Initializing st should not be necessary here. >>>> For example, perhaps lstat is a macro when compiling libsanitizer; if >>>> that is the underlying problem, then we should fix the macro, not this >>>> code. >>> Yeah, I'm not sure why the compiler thinks lstat isn't initializing st. >>> What should I do to debug this further? >> >> Guess you should start by telling us which OS it is on (I can't reproduce >> this warning on x86_64-linux nor i686-linux with glibc 2.28), looking at >> preprocessed source to see what exactly lstat does (e.g. if it is some macro >> or inline function and what exactly it is doing). > I am cross compiling with buildroot master branch using ubuntu 18.10. > I am building gcc 8.3.0 and glibc 2.29 for the cross toolchain. > The build and target systems are both x86_64. Add "-save-temps" to the command line. That will create a .i file, send the .i file along with the command line. jeff
On Mon, Mar 18, 2019 at 5:46 PM Jeff Law <law@redhat.com> wrote: > > On 3/18/19 5:07 PM, James Hilliard wrote: > > On Mon, Mar 18, 2019 at 4:51 PM Jakub Jelinek <jakub@redhat.com> wrote: > >> > >> On Mon, Mar 18, 2019 at 04:41:05PM -0600, James Hilliard wrote: > >>>> Thanks, but I'm saying that if you look at the code you can see that > >>>> st is clearly initialized, by the call to lstat. I would like to see > >>>> an explanation for why you are seeing that warning before changing the > >>>> code to disable it. Initializing st should not be necessary here. > >>>> For example, perhaps lstat is a macro when compiling libsanitizer; if > >>>> that is the underlying problem, then we should fix the macro, not this > >>>> code. > >>> Yeah, I'm not sure why the compiler thinks lstat isn't initializing st. > >>> What should I do to debug this further? > >> > >> Guess you should start by telling us which OS it is on (I can't reproduce > >> this warning on x86_64-linux nor i686-linux with glibc 2.28), looking at > >> preprocessed source to see what exactly lstat does (e.g. if it is some macro > >> or inline function and what exactly it is doing). > > I am cross compiling with buildroot master branch using ubuntu 18.10. > > I am building gcc 8.3.0 and glibc 2.29 for the cross toolchain. > > The build and target systems are both x86_64. > Add "-save-temps" to the command line. That will create a .i file, send > the .i file along with the command line. I added --save-temps to the cflags for the gcc package build. Here's the command line log: https://gist.githubusercontent.com/jameshilliard/015d972c26d1ec7bbbd1ad2d57d5dd3b/raw/7c848cdfba0967a681401de24ed6f4c86315d2d9/cli.log Here's the elf.i fille: https://gist.githubusercontent.com/jameshilliard/2a391d7292f3ba7412fe80e166c0f0e3/raw/63eb16ba9eff0b3ee668b16257b0948b2ca894a0/elf.i > > jeff
On 3/18/19 6:35 PM, James Hilliard wrote: > On Mon, Mar 18, 2019 at 5:46 PM Jeff Law <law@redhat.com> wrote: >> >> On 3/18/19 5:07 PM, James Hilliard wrote: >>> On Mon, Mar 18, 2019 at 4:51 PM Jakub Jelinek <jakub@redhat.com> wrote: >>>> >>>> On Mon, Mar 18, 2019 at 04:41:05PM -0600, James Hilliard wrote: >>>>>> Thanks, but I'm saying that if you look at the code you can see that >>>>>> st is clearly initialized, by the call to lstat. I would like to see >>>>>> an explanation for why you are seeing that warning before changing the >>>>>> code to disable it. Initializing st should not be necessary here. >>>>>> For example, perhaps lstat is a macro when compiling libsanitizer; if >>>>>> that is the underlying problem, then we should fix the macro, not this >>>>>> code. >>>>> Yeah, I'm not sure why the compiler thinks lstat isn't initializing st. >>>>> What should I do to debug this further? >>>> >>>> Guess you should start by telling us which OS it is on (I can't reproduce >>>> this warning on x86_64-linux nor i686-linux with glibc 2.28), looking at >>>> preprocessed source to see what exactly lstat does (e.g. if it is some macro >>>> or inline function and what exactly it is doing). >>> I am cross compiling with buildroot master branch using ubuntu 18.10. >>> I am building gcc 8.3.0 and glibc 2.29 for the cross toolchain. >>> The build and target systems are both x86_64. >> Add "-save-temps" to the command line. That will create a .i file, send >> the .i file along with the command line. > I added --save-temps to the cflags for the gcc package build. > Here's the command line log: > https://gist.githubusercontent.com/jameshilliard/015d972c26d1ec7bbbd1ad2d57d5dd3b/raw/7c848cdfba0967a681401de24ed6f4c86315d2d9/cli.log > Here's the elf.i fille: > https://gist.githubusercontent.com/jameshilliard/2a391d7292f3ba7412fe80e166c0f0e3/raw/63eb16ba9eff0b3ee668b16257b0948b2ca894a0/elf.i Jakub -- I haven't looked at it in any detail, but the first thing that jumps out at me is the -Og and the message: > ../../../../libsanitizer/libbacktrace/../../libbacktrace/elf.c: In function ‘elf_is_symlink’: > ../../../../libsanitizer/libbacktrace/../../libbacktrace/elf.c:772:21: error: ‘st.st_mode’ may be used uninitialized in this function [-Werror=maybe-uninitialized] > return S_ISLNK (st.st_mode); It could be the issues we've see with SRA and aggregates that have led to the discussion around breaking those into a distinct class of uninit warnings because we don't handle them well. I wouldn't be at all surprised if it goes away at -O1 or -O2. jeff
On Mon, Mar 18, 2019 at 6:58 PM Jeff Law <law@redhat.com> wrote: > > On 3/18/19 6:35 PM, James Hilliard wrote: > > On Mon, Mar 18, 2019 at 5:46 PM Jeff Law <law@redhat.com> wrote: > >> > >> On 3/18/19 5:07 PM, James Hilliard wrote: > >>> On Mon, Mar 18, 2019 at 4:51 PM Jakub Jelinek <jakub@redhat.com> wrote: > >>>> > >>>> On Mon, Mar 18, 2019 at 04:41:05PM -0600, James Hilliard wrote: > >>>>>> Thanks, but I'm saying that if you look at the code you can see that > >>>>>> st is clearly initialized, by the call to lstat. I would like to see > >>>>>> an explanation for why you are seeing that warning before changing the > >>>>>> code to disable it. Initializing st should not be necessary here. > >>>>>> For example, perhaps lstat is a macro when compiling libsanitizer; if > >>>>>> that is the underlying problem, then we should fix the macro, not this > >>>>>> code. > >>>>> Yeah, I'm not sure why the compiler thinks lstat isn't initializing st. > >>>>> What should I do to debug this further? > >>>> > >>>> Guess you should start by telling us which OS it is on (I can't reproduce > >>>> this warning on x86_64-linux nor i686-linux with glibc 2.28), looking at > >>>> preprocessed source to see what exactly lstat does (e.g. if it is some macro > >>>> or inline function and what exactly it is doing). > >>> I am cross compiling with buildroot master branch using ubuntu 18.10. > >>> I am building gcc 8.3.0 and glibc 2.29 for the cross toolchain. > >>> The build and target systems are both x86_64. > >> Add "-save-temps" to the command line. That will create a .i file, send > >> the .i file along with the command line. > > I added --save-temps to the cflags for the gcc package build. > > Here's the command line log: > > https://gist.githubusercontent.com/jameshilliard/015d972c26d1ec7bbbd1ad2d57d5dd3b/raw/7c848cdfba0967a681401de24ed6f4c86315d2d9/cli.log > > Here's the elf.i fille: > > https://gist.githubusercontent.com/jameshilliard/2a391d7292f3ba7412fe80e166c0f0e3/raw/63eb16ba9eff0b3ee668b16257b0948b2ca894a0/elf.i > Jakub -- I haven't looked at it in any detail, but the first thing that > jumps out at me is the -Og and the message: > > > ../../../../libsanitizer/libbacktrace/../../libbacktrace/elf.c: In function ‘elf_is_symlink’: > > ../../../../libsanitizer/libbacktrace/../../libbacktrace/elf.c:772:21: error: ‘st.st_mode’ may be used uninitialized in this function [-Werror=maybe-uninitialized] > > return S_ISLNK (st.st_mode); > > It could be the issues we've see with SRA and aggregates that have led > to the discussion around breaking those into a distinct class of uninit > warnings because we don't handle them well. I wouldn't be at all > surprised if it goes away at -O1 or -O2. I did a rebuild with -O2 and it worked. > > jeff
On Mon, Mar 18, 2019 at 06:35:12PM -0600, James Hilliard wrote: > On Mon, Mar 18, 2019 at 5:46 PM Jeff Law <law@redhat.com> wrote: > > > > On 3/18/19 5:07 PM, James Hilliard wrote: > > > On Mon, Mar 18, 2019 at 4:51 PM Jakub Jelinek <jakub@redhat.com> wrote: > > >> > > >> On Mon, Mar 18, 2019 at 04:41:05PM -0600, James Hilliard wrote: > > >>>> Thanks, but I'm saying that if you look at the code you can see that > > >>>> st is clearly initialized, by the call to lstat. I would like to see > > >>>> an explanation for why you are seeing that warning before changing the > > >>>> code to disable it. Initializing st should not be necessary here. > > >>>> For example, perhaps lstat is a macro when compiling libsanitizer; if > > >>>> that is the underlying problem, then we should fix the macro, not this > > >>>> code. > > >>> Yeah, I'm not sure why the compiler thinks lstat isn't initializing st. > > >>> What should I do to debug this further? > > >> > > >> Guess you should start by telling us which OS it is on (I can't reproduce > > >> this warning on x86_64-linux nor i686-linux with glibc 2.28), looking at > > >> preprocessed source to see what exactly lstat does (e.g. if it is some macro > > >> or inline function and what exactly it is doing). > > > I am cross compiling with buildroot master branch using ubuntu 18.10. > > > I am building gcc 8.3.0 and glibc 2.29 for the cross toolchain. > > > The build and target systems are both x86_64. > > Add "-save-temps" to the command line. That will create a .i file, send > > the .i file along with the command line. > I added --save-temps to the cflags for the gcc package build. > Here's the command line log: > https://gist.githubusercontent.com/jameshilliard/015d972c26d1ec7bbbd1ad2d57d5dd3b/raw/7c848cdfba0967a681401de24ed6f4c86315d2d9/cli.log > Here's the elf.i fille: > https://gist.githubusercontent.com/jameshilliard/2a391d7292f3ba7412fe80e166c0f0e3/raw/63eb16ba9eff0b3ee668b16257b0948b2ca894a0/elf.i I still can't reproduce it, even with gcc 8.3.0: /d/gcc-8.3.0/objdir/gcc/xgcc -B /d/gcc-8.3.0/objdir/gcc/ -S -g3 -Og elf.i -W -Wall -Wwrite-strings -Wmissing-format-attribute -Wcast-qual -Werror -Wstrict-prototypes -Wmissing-prototypes -Wold-style-definition -fPIC -Wno-implicit-fallthrough doesn't print anything. Jakub
On Tue, Mar 19, 2019 at 3:56 AM Jakub Jelinek <jakub@redhat.com> wrote: > > On Mon, Mar 18, 2019 at 06:35:12PM -0600, James Hilliard wrote: > > On Mon, Mar 18, 2019 at 5:46 PM Jeff Law <law@redhat.com> wrote: > > > > > > On 3/18/19 5:07 PM, James Hilliard wrote: > > > > On Mon, Mar 18, 2019 at 4:51 PM Jakub Jelinek <jakub@redhat.com> wrote: > > > >> > > > >> On Mon, Mar 18, 2019 at 04:41:05PM -0600, James Hilliard wrote: > > > >>>> Thanks, but I'm saying that if you look at the code you can see that > > > >>>> st is clearly initialized, by the call to lstat. I would like to see > > > >>>> an explanation for why you are seeing that warning before changing the > > > >>>> code to disable it. Initializing st should not be necessary here. > > > >>>> For example, perhaps lstat is a macro when compiling libsanitizer; if > > > >>>> that is the underlying problem, then we should fix the macro, not this > > > >>>> code. > > > >>> Yeah, I'm not sure why the compiler thinks lstat isn't initializing st. > > > >>> What should I do to debug this further? > > > >> > > > >> Guess you should start by telling us which OS it is on (I can't reproduce > > > >> this warning on x86_64-linux nor i686-linux with glibc 2.28), looking at > > > >> preprocessed source to see what exactly lstat does (e.g. if it is some macro > > > >> or inline function and what exactly it is doing). > > > > I am cross compiling with buildroot master branch using ubuntu 18.10. > > > > I am building gcc 8.3.0 and glibc 2.29 for the cross toolchain. > > > > The build and target systems are both x86_64. > > > Add "-save-temps" to the command line. That will create a .i file, send > > > the .i file along with the command line. > > I added --save-temps to the cflags for the gcc package build. > > Here's the command line log: > > https://gist.githubusercontent.com/jameshilliard/015d972c26d1ec7bbbd1ad2d57d5dd3b/raw/7c848cdfba0967a681401de24ed6f4c86315d2d9/cli.log > > Here's the elf.i fille: > > https://gist.githubusercontent.com/jameshilliard/2a391d7292f3ba7412fe80e166c0f0e3/raw/63eb16ba9eff0b3ee668b16257b0948b2ca894a0/elf.i > > I still can't reproduce it, even with gcc 8.3.0: > /d/gcc-8.3.0/objdir/gcc/xgcc -B /d/gcc-8.3.0/objdir/gcc/ -S -g3 -Og elf.i -W -Wall -Wwrite-strings -Wmissing-format-attribute -Wcast-qual -Werror -Wstrict-prototypes -Wmissing-prototypes -Wold-style-definition -fPIC -Wno-implicit-fallthrough > doesn't print anything. Try downloading buildroot(git commit 0af24710da70b04947229333c9450c86499b3108) and use this defconfig which should reproduce the problem: https://gist.github.com/jameshilliard/80e61f58e05e1325e0dbc6e30a11c851/raw/8e7d67cd1cc48f94b50e21f1ec407f29fe1dfe37/defconfig > > Jakub
diff --git a/libbacktrace/elf.c b/libbacktrace/elf.c index f3988ec02a0..714bfec965d 100644 --- a/libbacktrace/elf.c +++ b/libbacktrace/elf.c @@ -765,7 +765,7 @@ elf_syminfo (struct backtrace_state *state, uintptr_t addr, static int elf_is_symlink (const char *filename) { - struct stat st; + struct stat st={0}; if (lstat (filename, &st) < 0) return 0;
From: James Hilliard <james.hilliard1@gmail.com> Fixes error: ‘st.st_mode’ may be used uninitialized in this function --- libbacktrace/elf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)