From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 127910 invoked by alias); 25 Oct 2016 18:16:02 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 127690 invoked by uid 89); 25 Oct 2016 18:16:01 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 25 Oct 2016 18:15:59 +0000 Received: from svr-orw-mbx-03.mgc.mentorg.com ([147.34.90.203]) by relay1.mentorg.com with esmtp id 1bz6GT-0000m7-Df from Luis_Gustavo@mentor.com ; Tue, 25 Oct 2016 11:15:57 -0700 Received: from [172.30.1.187] (147.34.91.1) by svr-orw-mbx-03.mgc.mentorg.com (147.34.90.203) with Microsoft SMTP Server (TLS) id 15.0.1210.3; Tue, 25 Oct 2016 11:15:54 -0700 Reply-To: Luis Machado Subject: Re: [PATCH, v4] PR 20569, segv in follow_exec References: <1477415521-22010-1-git-send-email-lgustavo@codesourcery.com> <645baeb5-e813-d97d-fcd0-d0c7ea7bf5ba@redhat.com> To: Pedro Alves , CC: From: Luis Machado Message-ID: <3f775c2e-6519-40c3-78da-7da06a940bce@codesourcery.com> Date: Tue, 25 Oct 2016 18:16:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: <645baeb5-e813-d97d-fcd0-d0c7ea7bf5ba@redhat.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-ClientProxiedBy: svr-orw-mbx-04.mgc.mentorg.com (147.34.90.204) To svr-orw-mbx-03.mgc.mentorg.com (147.34.90.203) X-IsSubscribed: yes X-SW-Source: 2016-10/txt/msg00708.txt.bz2 On 10/25/2016 01:04 PM, Pedro Alves wrote: > On 10/25/2016 06:12 PM, Luis Machado wrote: >> Adjusted a few more bits. >> >> I ended up using the ugly explicit cast for an enum that is being used as a >> bitfield. I'm guessing we'll want to convert this to the macro-fied >> template-fied version that is more appropriate to C++? > > Yup. I actually have a patch for that in one of my C++ branches > somewhere. > >> >> If so then i can come up with a follow up patch that does the conversion. I >> didn't want to disrupt v4 with more unrelated changes. > > Use 'int' in this patch like the rest of the functions that pass > around this enum, and remove the casts. Then when the enum > is made a proper enum-flags with DEF_ENUM_FLAGS_TYPE, you won't > have to remove the casts then, and all functions that need adjustment > will get the same adjustment at the same time. No need for a partial > transition. > >> > >> void >> -exec_file_locate_attach (int pid, int defer_bp_reset, int from_tty) >> +try_open_exec_file (const char *exec_file_host, struct inferior *inf, >> + enum symfile_add_flags flags, int from_tty) > > Use int. > >> { >> - char *exec_file, *full_exec_path = NULL; >> struct cleanup *old_chain; >> struct gdb_exception prev_err = exception_none; >> >> - /* Do nothing if we already have an executable filename. */ >> - exec_file = (char *) get_exec_file (0); >> - if (exec_file != NULL) >> - return; >> - >> - /* Try to determine a filename from the process itself. */ >> - exec_file = target_pid_to_exec_file (pid); >> - if (exec_file == NULL) >> - { >> - warning (_("No executable has been specified and target does not " >> - "support\n" >> - "determining executable automatically. " >> - "Try using the \"file\" command.")); >> - return; >> - } >> - >> - /* If gdb_sysroot is not empty and the discovered filename >> - is absolute then prefix the filename with gdb_sysroot. */ >> - if (*gdb_sysroot != '\0' && IS_ABSOLUTE_PATH (exec_file)) >> - { >> - full_exec_path = exec_file_find (exec_file, NULL); >> - if (full_exec_path == NULL) >> - return; >> - } >> - else >> - { >> - /* It's possible we don't have a full path, but rather just a >> - filename. Some targets, such as HP-UX, don't provide the >> - full path, sigh. >> - >> - Attempt to qualify the filename against the source path. >> - (If that fails, we'll just fall back on the original >> - filename. Not much more we can do...) */ >> - if (!source_full_path_of (exec_file, &full_exec_path)) >> - full_exec_path = xstrdup (exec_file); >> - } >> - >> - old_chain = make_cleanup (xfree, full_exec_path); >> - make_cleanup (free_current_contents, &prev_err.message); >> + old_chain = make_cleanup (free_current_contents, &prev_err.message); >> >> /* exec_file_attach and symbol_file_add_main may throw an error if the file >> cannot be opened either locally or remotely. >> @@ -217,7 +160,9 @@ exec_file_locate_attach (int pid, int defer_bp_reset, int from_tty) >> errors/exceptions in the following code. */ >> TRY >> { >> - exec_file_attach (full_exec_path, from_tty); >> + /* We must do this step even if exec_file_host is NULL, so that >> + exec_file_attach will clear state. */ >> + exec_file_attach (exec_file_host, from_tty); >> } >> CATCH (err, RETURN_MASK_ERROR) >> { >> @@ -232,20 +177,64 @@ exec_file_locate_attach (int pid, int defer_bp_reset, int from_tty) >> } >> END_CATCH >> >> - TRY >> + if (exec_file_host != NULL) >> { >> - if (defer_bp_reset) >> - current_inferior ()->symfile_flags |= SYMFILE_DEFER_BP_RESET; >> - symbol_file_add_main (full_exec_path, from_tty); >> + /* SYMFILE_DEFER_BP_RESET is used as the proper displacement for PIE >> + (Position Independent Executable) main symbol file will get applied by >> + solib_create_inferior_hook below. breakpoint_re_set would fail to insert >> + the breakpoints with the zero displacement. */ >> + TRY > > I don't understand how it makes sense to this comment _here_. > Move this to where the flag is first passed down. Note how there's > no "solib_create_inferior_hook below" at all here. This function > just passes down the "flags" argument unmodified. > >> + { >> + symbol_file_add_main_1 (exec_file_host, from_tty, flags); >> + >> + if ((inf->symfile_flags & SYMFILE_NO_READ) == 0) >> + set_initial_language (); > > This is already done by symbol_file_add_main_1.. > >> + } >> + CATCH (err, RETURN_MASK_ERROR) >> + { >> + if (!exception_print_same (prev_err, err)) >> + warning ("%s", err.message); >> + } >> + END_CATCH >> + >> + inf->symfile_flags &= ~SYMFILE_DEFER_BP_RESET; > > Before the patch, inf->symfile_flags is enabled/disabled > around the body of the code. But now you pass down > SYMFILE_DEFER_BP_RESET in 'flags'. I think we should > be able to remove this? > >> } >> - CATCH (err, RETURN_MASK_ERROR) >> + >> + do_cleanups (old_chain); >> +} >> + >> +/* See gdbcore.h. */ >> + >> +void >> +exec_file_locate_attach (int pid, int defer_bp_reset, int from_tty) >> +{ >> + char *exec_file_target, *exec_file_host; >> + struct cleanup *old_chain; >> + symfile_add_flags flags = SYMFILE_MAINLINE; > > Use int for now. Start with 0. try_open_exec_file > always adds the SYMFILE_MAINLINE for you. > >> + >> + /* Do nothing if we already have an executable filename. */ >> + if (get_exec_file (0) != NULL) >> + return; >> + >> + /* Try to determine a filename from the process itself. */ >> + exec_file_target = target_pid_to_exec_file (pid); >> + if (exec_file_target == NULL) >> { >> - if (!exception_print_same (prev_err, err)) >> - warning ("%s", err.message); >> + warning (_("No executable has been specified and target does not " >> + "support\n" >> + "determining executable automatically. " >> + "Try using the \"file\" command.")); >> + return; >> } >> - END_CATCH >> - current_inferior ()->symfile_flags &= ~SYMFILE_DEFER_BP_RESET; >> >> + exec_file_host = exec_file_find (exec_file_target, NULL); >> + old_chain = make_cleanup (xfree, exec_file_host); >> + >> + if (defer_bp_reset) >> + flags = (enum symfile_add_flags) (flags | SYMFILE_DEFER_BP_RESET); > > Now with int, you can write: > > flags |= SYMFILE_DEFER_BP_RESET; > >> gdb_assert (current_program_space == inf->pspace); >> >> - /* That a.out is now the one to use. */ >> - exec_file_attach (execd_pathname, 0); >> + /* Attempt to open the exec file. */ >> + try_open_exec_file (exec_file_host, inf, (enum symfile_add_flags) 0, 0); > > Hmm, doesn't thi lose the SYMFILE_DEFER_BP_RESET described ... > >> >> - /* SYMFILE_DEFER_BP_RESET is used as the proper displacement for PIE >> - (Position Independent Executable) main symbol file will get applied by >> - solib_create_inferior_hook below. breakpoint_re_set would fail to insert >> - the breakpoints with the zero displacement. */ >> - >> - symbol_file_add (execd_pathname, >> - (inf->symfile_flags >> - | SYMFILE_MAINLINE | SYMFILE_DEFER_BP_RESET), >> - NULL, 0); > > ... here, in the code that you're replacing? > > Seems to be that you should leave the comment here, and do: > > /* SYMFILE_DEFER_BP_RESET is used as the proper displacement for PIE > (Position Independent Executable) main symbol file will get applied by > solib_create_inferior_hook below. breakpoint_re_set would fail to insert > the breakpoints with the zero displacement. */ > try_open_exec_file (exec_file_host, inf, SYMFILE_DEFER_BP_RESET, 0); > > Here is where it makes sense to talk about the solib_create_inferior_hook > call below, because there's actually such a call below. > >> - >> - if ((inf->symfile_flags & SYMFILE_NO_READ) == 0) >> - set_initial_language (); >> + do_cleanups (old_chain); >> >> /* If the target can specify a description, read it. Must do this >> after flipping to the new executable (because the target supplied > > BTW, symbol_file_add_main_1 doesn't actually add "flags" to > its add_flags local, so it seems like we're actually losing > the passed down SYMFILE_DEFER_BP_RESET? Or is there some confusion > regarding symfile_add_flags vs objfile flags here? Oh looks > like there is... > > I think I'll go dig up my enum-flags patch, and that will I think > help clear this up a lot. Before i go ahead and adjust this even more, what's your plan and ETA for the above? This is disturbing more code as we try to consolidade slightly different functions into a single one in order to make things a bit more clean. But i'm afraid this is besides the point of the original patch itself? I just want to understand what's the end goal, because the scope seems to be changing slightly with each iteration. :-) Thanks, Luis