Re: [PATCH] [libbacktrace] Initialize st in elf_is_symlink
On Mon, Mar 18, 2019 at 11:19 AM Ian Lance Taylor wrote:
>
> On Sun, Mar 17, 2019 at 6:22 PM wrote:
> >
> > From: James Hilliard
> >
> > 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
Re: [PATCH] [libbacktrace] Initialize st in elf_is_symlink
On Mon, Mar 18, 2019 at 2:05 PM Ian Lance Taylor wrote:
>
> On Mon, Mar 18, 2019 at 11:57 AM James Hilliard
> wrote:
> >
> > On Mon, Mar 18, 2019 at 11:19 AM Ian Lance Taylor wrote:
> > >
> > > On Sun, Mar 17, 2019 at 6:22 PM wrote:
> > > >
> > > > From: James Hilliard
> > > >
> > > > 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
Re: [PATCH] [libbacktrace] Initialize st in elf_is_symlink
On Mon, Mar 18, 2019 at 4:51 PM Jakub Jelinek 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
Re: [PATCH] [libbacktrace] Initialize st in elf_is_symlink
On Mon, Mar 18, 2019 at 5:46 PM Jeff Law wrote: > > On 3/18/19 5:07 PM, James Hilliard wrote: > > On Mon, Mar 18, 2019 at 4:51 PM Jakub Jelinek 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
Re: [PATCH] [libbacktrace] Initialize st in elf_is_symlink
On Mon, Mar 18, 2019 at 6:58 PM Jeff Law wrote: > > On 3/18/19 6:35 PM, James Hilliard wrote: > > On Mon, Mar 18, 2019 at 5:46 PM Jeff Law wrote: > >> > >> On 3/18/19 5:07 PM, James Hilliard wrote: > >>> On Mon, Mar 18, 2019 at 4:51 PM Jakub Jelinek 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
Re: [PATCH] [libbacktrace] Initialize st in elf_is_symlink
On Tue, Mar 19, 2019 at 3:56 AM Jakub Jelinek 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 wrote: > > > > > > On 3/18/19 5:07 PM, James Hilliard wrote: > > > > On Mon, Mar 18, 2019 at 4:51 PM Jakub Jelinek 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
