From patchwork Mon Feb 5 11:57:01 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Burgess X-Patchwork-Id: 25793 Received: (qmail 11524 invoked by alias); 5 Feb 2018 11:57:15 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 11475 invoked by uid 89); 5 Feb 2018 11:57:14 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy=filled X-HELO: mail-wr0-f174.google.com Received: from mail-wr0-f174.google.com (HELO mail-wr0-f174.google.com) (209.85.128.174) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 05 Feb 2018 11:57:09 +0000 Received: by mail-wr0-f174.google.com with SMTP id i56so29253667wra.7 for ; Mon, 05 Feb 2018 03:57:08 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=y4jjCMfGAwr/ClB3HWpbcHCza14/32Zt3EZtZFbQ3Ek=; b=rtaxZ4rpmEncePyQthFTSy1oZU5ybIr22H6nH8qJcIhBejBiLKYs9SapA+BNDo8VTp VjCS5qFk1UlwmVB60VfSItDgHssuw63Tacf16SGxSMbUowpn2ZBWmdUCG6ZTOdraUBC3 /eez2pXBhXAPfI9G2xYq3+XH4cVR1oZwz7noVxIHSKn+oyylRniTdSe2HXSruME46t8t rcaQi+m62gWKHEz2naxR6LLUuwr2l39GRtA9xfFDOggoV5LsqmP0CEg8CGsNTaegdNBD CYrDESPjXvXbSvFRH9K/d/V4uYnH6g0kCfNbz5IkBOxlr02S3Qi/c4iGkK/ECnW1FGxh b5iA== X-Gm-Message-State: AKwxytcVfOv/9ai1ouBxo+Yh9MrinPE3OYj7mlnrxCcvs9T1BB8k7US6 DQ59UMVXNkBOHxCTg5jJQdAG+6bA X-Google-Smtp-Source: AH8x2256q4sdUGcgDZiXxSjmdtfCCCsUZTosGjXTrXoLSBfJvpBLOZhDVaR6DRYWIeiTPBh2wNSYrA== X-Received: by 10.223.183.46 with SMTP id l46mr26499570wre.33.1517831826890; Mon, 05 Feb 2018 03:57:06 -0800 (PST) Received: from localhost ([109.144.16.101]) by smtp.gmail.com with ESMTPSA id b79sm4041006wmb.18.2018.02.05.03.57.04 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 05 Feb 2018 03:57:06 -0800 (PST) From: Andrew Burgess To: gdb-patches@sourceware.org Cc: Andrew Burgess Subject: [PATCH] sim: Don't overwrite stored errno in sim_syscall_multi Date: Mon, 5 Feb 2018 11:57:01 +0000 Message-Id: <20180205115701.21260-1-andrew.burgess@embecosm.com> X-IsSubscribed: yes The patch below hasn't been tested extensively, and not with any of the currently in tree simulators. I ran into an issue while working with an out of tree simulator where I found that in cases where syscalls were failing, the wrong errno was being reported. I tracked the issue down to what I think is a generic issue in the simulator (which is described in the commit message for the change below). I've posted the patch at this stage, as it might be useful to others. To move this forward, is there a specific set of simulator(s) that I should build against to test this change? My concern is that the code I'm removing might have originally been put in place because, somewhere, there is a code path where the syscall errno is not being stored correctly, and so catching the errno at a late phase was "fixing" this issue. If anyone wanted to merge this and try it against their simulators and report your test results that would be great. All feedback and suggestions welcome. Thanks, Andrew --- The host syscall callback mechanism should take care of updating the errcode within the CB_SYSCALL struct, and we should not be adjusting the error code once the syscall has completed. We especially, should not be rewriting the syscall errcode based on the value of errno some time after running the host syscall, as there is no guarantee that errno has not be overwritten. Lets consider why reading errno in sim_syscall_multi is a bad idea. To perform a syscall we call cb_syscall (in syscall.c), there are two exit paths from this function, these are called FinishSyscall and ErrorFinish. In FinishSyscall we store the syscall result in 'sc->result', and the error code is transated to target encoding, and stored in 'sc->errcode'. In ErrorFinish, we again store the syscall result in 'sc->result', and fill in 'sc->errcode' by fetching the actual errno from the host with the 'cb->get_errno' callback. In both cases 'sc->errcode' should have been filled in with an appropriate value. However, lets consider a specific syscall CB_SYS_open, in this case, the first thing we do is fetch the path to open from the target with 'get_path', if this fails then the errcode is returned, and we call FinishSyscall. Notice that in this case, no host syscall may have been performed, and so reading errno makes absolutely no sense. This commit removes from sim_syscall_multi the rewriting of sc->errcode based on the value of errno. sim/common/ChangeLog: * sim-syscall.c (sim_syscall_multi): Don't update sc->errcode at this point, it should have already been set in cb_syscall. --- sim/common/ChangeLog | 5 +++++ sim/common/sim-syscall.c | 5 ----- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/sim/common/sim-syscall.c b/sim/common/sim-syscall.c index 7923160cb7f..34d2f0ec115 100644 --- a/sim/common/sim-syscall.c +++ b/sim/common/sim-syscall.c @@ -97,11 +97,6 @@ sim_syscall_multi (SIM_CPU *cpu, int func, long arg1, long arg2, long arg3, if (cb_target_to_host_syscall (cb, func) == CB_SYS_exit) sim_engine_halt (sd, cpu, NULL, sim_pc_get (cpu), sim_exited, arg1); - else if (sc.result == -1) - { - cb->last_errno = errno; - sc.errcode = cb->get_errno (cb); - } *result = sc.result; *result2 = sc.result2;