Re: [PATCH 1/2] ivopts: Revert computation of address cost complexity.
Hi Richard, > The only documentation on complexity I find is > > int64_t cost; /* The runtime cost. */ > unsigned complexity; /* The estimate of the complexity of the code for > the computation (in no concrete units -- > complexity field should be larger for more > complex expressions and addressing modes). */ > > and complexity is used as tie-breaker only when cost is equal. Given that > shouldn't unsupported addressing modes have higher complexity? I'll note > that there's nothing "unsupported", each "unsupported" address computation > is lowered into supported pieces. "unsupported" maybe means that > "cost" isn't fully covered by address-cost and compensation stmts might > be costed in quantities not fully compatible with that? Correct, that's what I was aiming for initially - before f9f69dd that was the case, "unsupported" addressing modes had higher complexities. Also, that's what I meant by "unsupported" as well, thanks. > That said, "complexity" seems to only complicate things :/ We do have the > tie-breaker on preferring less IVs. complexity was added in > r0-85562-g6e8c65f6621fb0 as part of fixing PR34711. I agree that the complexity part is just (kind of) out there, not really strongly defined. I'm not sure how to feel about merging complexity into the cost part of an address cost, though. > If it's really only about the "complexity" value then each > compensation step should > add to the complexity? That could be the way to go. Also worth verifying is that we compensate for each case of an unsupported addressing mode. Kind regards, Dimitrije From: Richard Biener Sent: Friday, December 16, 2022 10:58 AM To: Dimitrije Milosevic Cc: Jeff Law ; gcc-patches@gcc.gnu.org ; Djordje Todorovic Subject: Re: [PATCH 1/2] ivopts: Revert computation of address cost complexity. On Thu, Dec 15, 2022 at 4:26 PM Dimitrije Milosevic wrote: > > Hi Richard, > > Sorry for the delayed response, I couldn't find the time to fully focus on > this topic. > > > I'm not sure this is accurate but at least the cost of using an unsupported > > addressing mode should be at least that of the compensating code to > > mangle it to a supported form. > > I'm pretty sure IVOPTS does not filter out candidates which aren't supported > by > the target architecture. It does, however, adjust the cost for a subset of > those. > The adjustment code modifies only the cost part of the address cost (which > consists of a cost and a complexity). > Having said this, I'd propose two approaches: > 1. Cover all cases of unsupported addressing modes (if needed, I'm not >entirely > sure they aren't already covered), leaving complexity for unsupported > addressing modes zero. The only documentation on complexity I find is int64_t cost; /* The runtime cost. */ unsigned complexity; /* The estimate of the complexity of the code for the computation (in no concrete units -- complexity field should be larger for more complex expressions and addressing modes). */ and complexity is used as tie-breaker only when cost is equal. Given that shouldn't unsupported addressing modes have higher complexity? I'll note that there's nothing "unsupported", each "unsupported" address computation is lowered into supported pieces. "unsupported" maybe means that "cost" isn't fully covered by address-cost and compensation stmts might be costed in quantities not fully compatible with that? That said, "complexity" seems to only complicate things :/ We do have the tie-breaker on prefering less IVs. complexity was added in r0-85562-g6e8c65f6621fb0 as part of fixing PR34711. > 2. Revert the complexity calculation (which my initial patch does), >leaving > everything else as it is. > 3. A combination of both - if the control path gets into the adjustment >code, we > use the reverted complexity calculation. If it's really only about the "complexity" value then each compensation step should add to the complexity? > I'd love to get feedback regarding this, so I could focus on a concrete > approach. > > Kind regards, > Dimitrije > > From: Richard Biener > Sent: Monday, November 7, 2022 2:35 PM > To: Dimitrije Milosevic > Cc: Jeff Law ; gcc-patches@gcc.gnu.org > ; Djordje Todorovic > Subject: Re: [PATCH 1/2] ivopts: Revert computation of address cost > complexity. > > On Wed, Nov 2, 2022 at 9:40 AM Dimitrije Milosevic > wrote: > > > > Hi Jeff, > > > > &
Re: [PATCH 1/2] ivopts: Revert computation of address cost complexity.
Hi Richard, Sorry for the delayed response, I couldn't find the time to fully focus on this topic. > I'm not sure this is accurate but at least the cost of using an unsupported > addressing mode should be at least that of the compensating code to > mangle it to a supported form. I'm pretty sure IVOPTS does not filter out candidates which aren't supported by the target architecture. It does, however, adjust the cost for a subset of those. The adjustment code modifies only the cost part of the address cost (which consists of a cost and a complexity). Having said this, I'd propose two approaches: 1. Cover all cases of unsupported addressing modes (if needed, I'm not entirely sure they aren't already covered), leaving complexity for unsupported addressing modes zero. 2. Revert the complexity calculation (which my initial patch does), leaving everything else as it is. 3. A combination of both - if the control path gets into the adjustment code, we use the reverted complexity calculation. I'd love to get feedback regarding this, so I could focus on a concrete approach. Kind regards, Dimitrije From: Richard Biener Sent: Monday, November 7, 2022 2:35 PM To: Dimitrije Milosevic Cc: Jeff Law ; gcc-patches@gcc.gnu.org ; Djordje Todorovic Subject: Re: [PATCH 1/2] ivopts: Revert computation of address cost complexity. On Wed, Nov 2, 2022 at 9:40 AM Dimitrije Milosevic wrote: > > Hi Jeff, > > > This is exactly what I was trying to get to. If the addressing mode > > isn't supported, then we shouldn't be picking it as a candidate. If it > > is, then we've probably got a problem somewhere else in this code and > > this patch is likely papering over it. I'm not sure this is accurate but at least the cost of using an unsupported addressing mode should be at least that of the compensating code to mangle it to a supported form. > I'll take a deeper look into the candidate selection algorithm then. Will > get back to you. Thanks - as said the unfortunate situation is that both the original author and the one who did the last bigger reworks of the code are gone. Richard. > Regards, > Dimitrije > > > From: Jeff Law > Sent: Tuesday, November 1, 2022 7:46 PM > To: Richard Biener; Dimitrije Milosevic > Cc: gcc-patches@gcc.gnu.org; Djordje Todorovic > Subject: Re: [PATCH 1/2] ivopts: Revert computation of address cost > complexity. > > > On 10/28/22 01:00, Richard Biener wrote: > > On Fri, Oct 28, 2022 at 8:43 AM Dimitrije Milosevic > > wrote: > >> Hi Jeff, > >> > >>> THe part I don't understand is, if you only have BASE+OFF, why does > >>> preventing the calculation of more complex addressing modes matter? ie, > >>> what's the point of computing the cost of something like base + off + > >>> scaled index when the target can't utilize it? > >> Well, the complexities of all addressing modes other than BASE + OFFSET are > >> equal to 0. For targets like Mips, which only has BASE + OFFSET, it would > >> still > >> be more complex to use a candidate with BASE + INDEX << SCALE + OFFSET > >> than a candidate with BASE + INDEX, for example, as it has to compensate > >> the lack of other addressing modes somehow. If complexities for both of > >> those are equal to 0, in cases where complexities decide which candidate is > >> to be chosen, a more complex candidate may be picked. > > But something is wrong then - it shouldn't ever pick a candidate with > > an addressing > > mode that isn't supported? So you say that the cost of expressing > > 'BASE + INDEX << SCALE + OFFSET' as 'BASE + OFFSET' is not computed > > accurately? > > This is exactly what I was trying to get to. If the addressing mode > isn't supported, then we shouldn't be picking it as a candidate. If it > is, then we've probably got a problem somewhere else in this code and > this patch is likely papering over it. > > > Jeff >
Re: [PATCH 1/2] ivopts: Revert computation of address cost complexity.
Hi Jeff, > This is exactly what I was trying to get to. If the addressing mode > isn't supported, then we shouldn't be picking it as a candidate. If it > is, then we've probably got a problem somewhere else in this code and > this patch is likely papering over it. I'll take a deeper look into the candidate selection algorithm then. Will get back to you. Regards, Dimitrije From: Jeff Law Sent: Tuesday, November 1, 2022 7:46 PM To: Richard Biener; Dimitrije Milosevic Cc: gcc-patches@gcc.gnu.org; Djordje Todorovic Subject: Re: [PATCH 1/2] ivopts: Revert computation of address cost complexity. On 10/28/22 01:00, Richard Biener wrote: > On Fri, Oct 28, 2022 at 8:43 AM Dimitrije Milosevic > wrote: >> Hi Jeff, >> >>> THe part I don't understand is, if you only have BASE+OFF, why does >>> preventing the calculation of more complex addressing modes matter? ie, >>> what's the point of computing the cost of something like base + off + >>> scaled index when the target can't utilize it? >> Well, the complexities of all addressing modes other than BASE + OFFSET are >> equal to 0. For targets like Mips, which only has BASE + OFFSET, it would >> still >> be more complex to use a candidate with BASE + INDEX << SCALE + OFFSET >> than a candidate with BASE + INDEX, for example, as it has to compensate >> the lack of other addressing modes somehow. If complexities for both of >> those are equal to 0, in cases where complexities decide which candidate is >> to be chosen, a more complex candidate may be picked. > But something is wrong then - it shouldn't ever pick a candidate with > an addressing > mode that isn't supported? So you say that the cost of expressing > 'BASE + INDEX << SCALE + OFFSET' as 'BASE + OFFSET' is not computed > accurately? This is exactly what I was trying to get to. If the addressing mode isn't supported, then we shouldn't be picking it as a candidate. If it is, then we've probably got a problem somewhere else in this code and this patch is likely papering over it. Jeff
Re: [PATCH 2/2] ivopts: Consider number of invariants when calculating register pressure.
Hi Richard, > It's n_invs + 2 * n_cands? Correct, n_invs + 2 * n_cands, my apologies. > The comment says we want to prefer eliminating IVs over invariants. Your > patch > undoes that by weighting invariants the same so it does no longer have > the effect > of the comment. I see how my patch may have confused you. My concern is the "If we have enough registers." case - if we do have enough registers to store both the invariants and induction variables, I think the cost should be equal to the sum of those. I understand that adding another n_cands could be used as a tie-breaker for the two cases where we do have enough registers and the sum of n_invs and n_cands is equal, however I think there are two problems with that: - How often does it happen that we have two cases where we do have enough registers, n_invs + n_cands sums are equal, and n_cands differ? I think that's pretty rare. - Bumping up the cost by another n_cands may lead to cost for the "If we do have enough registers." case to be higher than for other cases, which doesn't make sense. I can refer to the test case that I presented in [0] for the second point. Also worth noting is that the estimate_reg_pressure_cost function (used before c18101f) follows this: /* If we have enough registers, we should use them and not restrict the transformations unnecessarily. */ if (regs_needed + target_res_regs <= available_regs) return 0; As far as preferring to eliminate induction variables if possible, don't we already do that, for example: /* If the number of candidates runs out available registers, we penalize extra candidate registers using target_spill_cost * 2. Because it is more expensive to spill induction variable than invariant. */ else cost = target_reg_cost [speed] * available_regs + target_spill_cost [speed] * (n_cands - available_regs) * 2 + target_spill_cost [speed] * (regs_needed - n_cands); To clarify, what my patch did was that it gave every case a base cost of n_invs + n_cands. This base cost gets bumped up accordingly, for each one of the cases (by the amount equal to "cost = ..." statement prior to the return statement in the ivopts_estimate_reg_pressure function). I agree that my patch isn't clear on my intention, and that it also does not correspond to the comment. What I could do is just return n_new as the cost for the "If we do have enough registers." case, but I would love to hear your thoughts, if I clarified my intention a little bit. [0] https://gcc.gnu.org/pipermail/gcc-patches/2022-October/604304.html Regards, Dimitrije From: Richard Biener Sent: Friday, October 28, 2022 9:38 AM To: Dimitrije Milosevic Cc: gcc-patches@gcc.gnu.org ; Djordje Todorovic Subject: Re: [PATCH 2/2] ivopts: Consider number of invariants when calculating register pressure. On Tue, Oct 25, 2022 at 3:00 PM Dimitrije Milosevic wrote: > > Hi Richard, > > > don't you add n_invs twice now given > > > > unsigned n_old = data->regs_used, n_new = n_invs + n_cands; > > unsigned regs_needed = n_new + n_old, available_regs = target_avail_regs; > > > > ? > > If you are referring to the "If we have enough registers." case, correct. > After c18101f, > for that case, the returned cost is equal to 2 * n_invs + n_cands. It's n_invs + 2 * n_cands? And the comment states the reasoning. Before c18101f, for > that case, the returned cost is equal to n_invs + n_cands. Another solution > would be > to just return n_invs + n_cands if we have enough registers. The comment says we want to prefer eliminating IVs over invariants. Your patch undoes that by weighting invariants the same so it does no longer have the effect of the comment. > Regards, > Dimitrije > > > From: Richard Biener > Sent: Tuesday, October 25, 2022 1:07 PM > To: Dimitrije Milosevic > Cc: gcc-patches@gcc.gnu.org ; Djordje Todorovic > > Subject: Re: [PATCH 2/2] ivopts: Consider number of invariants when > calculating register pressure. > > On Fri, Oct 21, 2022 at 3:57 PM Dimitrije Milosevic > wrote: > > > > From: Dimitrije Milošević > > > > This patch slightly modifies register pressure model function to consider > > both the number of invariants and the number of candidates, rather than > > just the number of candidates. This used to be the case before c18101f. > > don't you add n_invs twice now given > > unsigned n_old = data->regs_used, n_new = n_invs + n_cands; > unsigned regs_needed = n_new + n_old, available_regs = target_avail_regs; > > ? > > > gcc/ChangeLog: > > > > * tree-ssa-loop-ivopts.cc (ivopts_estimate_reg_pressure): Adjust. > > > > Signed-off-by: Dimitrije Milosevic > > --- > > g
Re: [PATCH 1/2] ivopts: Revert computation of address cost complexity.
Hi Richard, > But something is wrong then - it shouldn't ever pick a candidate with > an addressing > mode that isn't supported? Test case I presented in [0] only has non-"BASE + OFFSET" candidates. Correct me if I'm wrong, but the candidate selection algorithm doesn't take into account which addressing modes are supported by the target? > So you say that the cost of expressing > 'BASE + INDEX << SCALE + OFFSET' as 'BASE + OFFSET' is not computed > accurately? I just took it as an example, but yes. > The function tries to compensate for that, maybe you can point out > where it goes wrong? > That is, at the end it adjusts cost and complexity based on what it > scrapped before, maybe > that is just a bit incomplete? I think the cost.cost part is mostly okay, as the costs are just scaled (what was lesser/higher before f9f69dd is lesser/higher after f9f69dd). As far as the adjustments go, I don't think they are complete. On the other hand, as complexity is a valid part of address costs, and it can be used as a tie-breaker, I feel like it should serve a purpose, even for targets like Mips which are limited when it comes to addressing modes, rather than being equal to 0. I guess an alternative would be to fully cover cost.cost adjustments, and leave the complexities to be 0 for non-supported addressing modes. Currently, they are implemented as follows: if (simple_inv) simple_inv = (aff_inv == NULL || aff_combination_const_p (aff_inv) || aff_combination_singleton_var_p (aff_inv)); if (!aff_combination_zero_p (aff_inv)) comp_inv = aff_combination_to_tree (aff_inv); if (comp_inv != NULL_TREE) cost = force_var_cost (data, comp_inv, inv_vars); if (ratio != 1 && parts.step == NULL_TREE) var_cost += mult_by_coeff_cost (ratio, addr_mode, speed); if (comp_inv != NULL_TREE && parts.index == NULL_TREE) var_cost += add_cost (speed, addr_mode); > Note the original author of this is not available so it would help > (maybe also yourself) to > walk through the function with a specific candidate / use where you > think the complexity > (or cost) is wrong? I'd like to refer to [0] where candidate costs didn't get adjusted to cover the lack of complexity calculation. Would love to hear your thoughts. [0] https://gcc.gnu.org/pipermail/gcc-patches/2022-October/604304.html Regards, Dimitrije From: Richard Biener Sent: Friday, October 28, 2022 9:00 AM To: Dimitrije Milosevic Cc: Jeff Law ; gcc-patches@gcc.gnu.org ; Djordje Todorovic Subject: Re: [PATCH 1/2] ivopts: Revert computation of address cost complexity. On Fri, Oct 28, 2022 at 8:43 AM Dimitrije Milosevic wrote: > > Hi Jeff, > > > THe part I don't understand is, if you only have BASE+OFF, why does > > preventing the calculation of more complex addressing modes matter? ie, > > what's the point of computing the cost of something like base + off + > > scaled index when the target can't utilize it? > > Well, the complexities of all addressing modes other than BASE + OFFSET are > equal to 0. For targets like Mips, which only has BASE + OFFSET, it would > still > be more complex to use a candidate with BASE + INDEX << SCALE + OFFSET > than a candidate with BASE + INDEX, for example, as it has to compensate > the lack of other addressing modes somehow. If complexities for both of > those are equal to 0, in cases where complexities decide which candidate is > to be chosen, a more complex candidate may be picked. But something is wrong then - it shouldn't ever pick a candidate with an addressing mode that isn't supported? So you say that the cost of expressing 'BASE + INDEX << SCALE + OFFSET' as 'BASE + OFFSET' is not computed accurately? The function tries to compensate for that, maybe you can point out where it goes wrong? That is, at the end it adjusts cost and complexity based on what it scrapped before, maybe that is just a bit incomplete? Note the original author of this is not available so it would help (maybe also yourself) to walk through the function with a specific candidate / use where you think the complexity (or cost) is wrong? > Regards, > Dimitrije > > > From: Jeff Law > Sent: Friday, October 28, 2022 1:02 AM > To: Dimitrije Milosevic ; > gcc-patches@gcc.gnu.org > Cc: Djordje Todorovic > Subject: Re: [PATCH 1/2] ivopts: Revert computation of address cost > complexity. > > > On 10/21/22 07:52, Dimitrije Milosevic wrote: > > From: Dimitrije Milošević > > > > This patch reverts the computation of address cost complexity > > to the legacy one. After f9f69dd, complexity is calculated > > using the valid_mem_ref_p target hook. Architectures like > > Mips only allow BASE + OFFSET addressing modes, which in turn > > prevent
Re: [PATCH 1/2] ivopts: Revert computation of address cost complexity.
Hi Jeff, > THe part I don't understand is, if you only have BASE+OFF, why does > preventing the calculation of more complex addressing modes matter? ie, > what's the point of computing the cost of something like base + off + > scaled index when the target can't utilize it? Well, the complexities of all addressing modes other than BASE + OFFSET are equal to 0. For targets like Mips, which only has BASE + OFFSET, it would still be more complex to use a candidate with BASE + INDEX << SCALE + OFFSET than a candidate with BASE + INDEX, for example, as it has to compensate the lack of other addressing modes somehow. If complexities for both of those are equal to 0, in cases where complexities decide which candidate is to be chosen, a more complex candidate may be picked. Regards, Dimitrije From: Jeff Law Sent: Friday, October 28, 2022 1:02 AM To: Dimitrije Milosevic ; gcc-patches@gcc.gnu.org Cc: Djordje Todorovic Subject: Re: [PATCH 1/2] ivopts: Revert computation of address cost complexity. On 10/21/22 07:52, Dimitrije Milosevic wrote: > From: Dimitrije Milošević > > This patch reverts the computation of address cost complexity > to the legacy one. After f9f69dd, complexity is calculated > using the valid_mem_ref_p target hook. Architectures like > Mips only allow BASE + OFFSET addressing modes, which in turn > prevents the calculation of complexity for other addressing > modes, resulting in non-optimal candidate selection. > > gcc/ChangeLog: > > * tree-ssa-address.cc (multiplier_allowed_in_address_p): Change > to non-static. > * tree-ssa-address.h (multiplier_allowed_in_address_p): Declare. > * tree-ssa-loop-ivopts.cc (compute_symbol_and_var_present): >Reintroduce. > (compute_min_and_max_offset): Likewise. > (get_address_cost): Revert > complexity calculation. THe part I don't understand is, if you only have BASE+OFF, why does preventing the calculation of more complex addressing modes matter? ie, what's the point of computing the cost of something like base + off + scaled index when the target can't utilize it? jeff
Re: [PATCH 1/2] ivopts: Revert computation of address cost complexity.
)
lwc1$f15,8($3)
maddf.s $f6,$f0,$f15
swc1$f6,8($2)
lwc1$f16,12($3)
maddf.s $f9,$f0,$f16
swc1$f9,12($2)
lwc1$f17,16($3)
maddf.s $f10,$f0,$f17
swc1$f10,16($2)
lwc1$f18,20($3)
maddf.s $f11,$f0,$f18
swc1$f11,20($2)
lwc1$f19,24($3)
maddf.s $f12,$f0,$f19
swc1$f12,24($2)
lwc1$f20,28($3)
maddf.s $f13,$f0,$f20
swc1$f13,28($2)
daddiu $2,$2,32
bne $2,$12,.L83
daddiu $3,$3,32
...
= Before f9f69dd =
= After f9f69dd =
.L93:
dsubu $18,$2,$4
lwc1$f13,0($2)
daddu $19,$18,$5
daddiu $16,$2,4
lwc1$f14,0($19)
dsubu $17,$16,$4
daddu $25,$17,$5
lwc1$f15,4($2)
daddiu $19,$2,12
daddiu $20,$2,8
maddf.s $f13,$f1,$f14
dsubu $16,$19,$4
daddiu $17,$2,16
dsubu $18,$20,$4
daddu $19,$16,$5
daddiu $16,$2,20
lwc1$f10,8($2)
daddu $20,$18,$5
lwc1$f16,12($2)
dsubu $18,$17,$4
lwc1$f17,16($2)
dsubu $17,$16,$4
lwc1$f18,20($2)
daddiu $16,$2,24
lwc1$f20,24($2)
daddu $18,$18,$5
swc1$f13,0($2)
daddu $17,$17,$5
lwc1$f19,0($25)
daddiu $25,$2,28
lwc1$f11,28($2)
daddiu $2,$2,32
dsubu $16,$16,$4
dsubu $25,$25,$4
maddf.s $f15,$f1,$f19
daddu $16,$16,$5
daddu $25,$25,$5
swc1$f15,-28($2)
lwc1$f21,0($20)
ld $20,48($sp)
maddf.s $f10,$f1,$f21
swc1$f10,-24($2)
lwc1$f22,0($19)
maddf.s $f16,$f1,$f22
swc1$f16,-20($2)
lwc1$f23,0($18)
maddf.s $f17,$f1,$f23
swc1$f17,-16($2)
lwc1$f0,0($17)
maddf.s $f18,$f1,$f0
swc1$f18,-12($2)
lwc1$f7,0($16)
maddf.s $f20,$f1,$f7
swc1$f20,-8($2)
lwc1$f12,0($25)
maddf.s $f11,$f1,$f12
bne $2,$20,.L93
swc1$f11,-4($2)
...
= After f9f69dd =
Notice the additional instructions used for index calculation, due to
unoptimal candidate selection.
Regards,
Dimitrije
From: Richard Biener
Sent: Tuesday, October 25, 2022 1:08 PM
To: Dimitrije Milosevic
Cc: gcc-patches@gcc.gnu.org ; Djordje Todorovic
Subject: Re: [PATCH 1/2] ivopts: Revert computation of address cost complexity.
On Fri, Oct 21, 2022 at 3:56 PM Dimitrije Milosevic
wrote:
>
> From: Dimitrije Milošević
>
> This patch reverts the computation of address cost complexity
> to the legacy one. After f9f69dd, complexity is calculated
> using the valid_mem_ref_p target hook. Architectures like
> Mips only allow BASE + OFFSET addressing modes, which in turn
> prevents the calculation of complexity for other addressing
> modes, resulting in non-optimal candidate selection.
I don't follow how only having BASE + OFFSET addressing prevents
calculation of complexity for other addressing modes? Can you explain?
Do you have a testcase that shows how both changes improve IV selection
for MIPS?
>
> gcc/ChangeLog:
>
> * tree-ssa-address.cc (multiplier_allowed_in_address_p): Change
> to non-static.
> * tree-ssa-address.h (multiplier_allowed_in_address_p): Declare.
> * tree-ssa-loop-ivopts.cc (compute_symbol_and_var_present):
>Reintroduce.
> (compute_min_and_max_offset): Likewise.
> (get_address_cost): Revert
> complexity calculation.
>
> Signed-off-by: Dimitrije Milosevic
> ---
> gcc/tree-ssa-address.cc | 2 +-
> gcc/tree-ssa-address.h | 2 +
> gcc/tree-ssa-loop-ivopts.cc | 214 ++--
> 3 files changed, 207 insertions(+), 11 deletions(-)
>
> diff --git a/gcc/tree-ssa-address.cc b/gcc/tree-ssa-address.cc
> index ba7b7c93162..442f54f0165 100644
> --- a/gcc/tree-ssa-address.cc
> +++ b/gcc/tree-ssa-address.cc
> @@ -561,7 +561,7 @@ add_to_parts (struct mem_address *parts, tree elt)
> validity for a memory reference accessing memory of mode MODE in address
> space AS. */
>
> -static bool
> +bool
> multiplier_allowed_in_address_p (HOST_WIDE_INT ratio, machine_mode mode,
> addr_space_t as)
> {
> diff --git a/gcc/tree-ssa-address.h b/gcc/tree-ssa-address.h
> index 95143a099b9..09f36ee2f19 100644
> --- a/gcc/tree-ssa-address.h
> +++ b/gcc/tree-ssa-address.h
> @@ -38,6 +38,8 @@ tree create_mem_ref (gimple_stmt_iterator *, tree,
> class aff_tree *, tree, tree, tree, bool);
> extern void copy_ref_info (tree, tree);
> tree maybe_fold_tmr
Re: [PATCH 2/2] ivopts: Consider number of invariants when calculating register pressure.
Hi Richard, > don't you add n_invs twice now given > > unsigned n_old = data->regs_used, n_new = n_invs + n_cands; > unsigned regs_needed = n_new + n_old, available_regs = target_avail_regs; > > ? If you are referring to the "If we have enough registers." case, correct. After c18101f, for that case, the returned cost is equal to 2 * n_invs + n_cands. Before c18101f, for that case, the returned cost is equal to n_invs + n_cands. Another solution would be to just return n_invs + n_cands if we have enough registers. Regards, Dimitrije From: Richard Biener Sent: Tuesday, October 25, 2022 1:07 PM To: Dimitrije Milosevic Cc: gcc-patches@gcc.gnu.org ; Djordje Todorovic Subject: Re: [PATCH 2/2] ivopts: Consider number of invariants when calculating register pressure. On Fri, Oct 21, 2022 at 3:57 PM Dimitrije Milosevic wrote: > > From: Dimitrije Milošević > > This patch slightly modifies register pressure model function to consider > both the number of invariants and the number of candidates, rather than > just the number of candidates. This used to be the case before c18101f. don't you add n_invs twice now given unsigned n_old = data->regs_used, n_new = n_invs + n_cands; unsigned regs_needed = n_new + n_old, available_regs = target_avail_regs; ? > gcc/ChangeLog: > > * tree-ssa-loop-ivopts.cc (ivopts_estimate_reg_pressure): Adjust. > > Signed-off-by: Dimitrije Milosevic > --- > gcc/tree-ssa-loop-ivopts.cc | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/gcc/tree-ssa-loop-ivopts.cc b/gcc/tree-ssa-loop-ivopts.cc > index d53ba05a4f6..9d0b669d671 100644 > --- a/gcc/tree-ssa-loop-ivopts.cc > +++ b/gcc/tree-ssa-loop-ivopts.cc > @@ -6409,9 +6409,9 @@ ivopts_estimate_reg_pressure (struct ivopts_data *data, > unsigned n_invs, > + target_spill_cost [speed] * (n_cands - available_regs) * 2 > + target_spill_cost [speed] * (regs_needed - n_cands); > > - /* Finally, add the number of candidates, so that we prefer eliminating > - induction variables if possible. */ > - return cost + n_cands; > + /* Finally, add the number of invariants and the number of candidates, > + so that we prefer eliminating induction variables if possible. */ > + return cost + n_invs + n_cands; > } > > /* For each size of the induction variable set determine the penalty. */ > -- > 2.25.1 >
[PATCH 2/2] ivopts: Consider number of invariants when calculating register pressure.
From: Dimitrije Milošević This patch slightly modifies register pressure model function to consider both the number of invariants and the number of candidates, rather than just the number of candidates. This used to be the case before c18101f. gcc/ChangeLog: * tree-ssa-loop-ivopts.cc (ivopts_estimate_reg_pressure): Adjust. Signed-off-by: Dimitrije Milosevic --- gcc/tree-ssa-loop-ivopts.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/gcc/tree-ssa-loop-ivopts.cc b/gcc/tree-ssa-loop-ivopts.cc index d53ba05a4f6..9d0b669d671 100644 --- a/gcc/tree-ssa-loop-ivopts.cc +++ b/gcc/tree-ssa-loop-ivopts.cc @@ -6409,9 +6409,9 @@ ivopts_estimate_reg_pressure (struct ivopts_data *data, unsigned n_invs, + target_spill_cost [speed] * (n_cands - available_regs) * 2 + target_spill_cost [speed] * (regs_needed - n_cands); - /* Finally, add the number of candidates, so that we prefer eliminating - induction variables if possible. */ - return cost + n_cands; + /* Finally, add the number of invariants and the number of candidates, + so that we prefer eliminating induction variables if possible. */ + return cost + n_invs + n_cands; } /* For each size of the induction variable set determine the penalty. */ -- 2.25.1
[PATCH 1/2] ivopts: Revert computation of address cost complexity.
From: Dimitrije Milošević
This patch reverts the computation of address cost complexity
to the legacy one. After f9f69dd, complexity is calculated
using the valid_mem_ref_p target hook. Architectures like
Mips only allow BASE + OFFSET addressing modes, which in turn
prevents the calculation of complexity for other addressing
modes, resulting in non-optimal candidate selection.
gcc/ChangeLog:
* tree-ssa-address.cc (multiplier_allowed_in_address_p): Change
to non-static.
* tree-ssa-address.h (multiplier_allowed_in_address_p): Declare.
* tree-ssa-loop-ivopts.cc (compute_symbol_and_var_present): Reintroduce.
(compute_min_and_max_offset): Likewise.
(get_address_cost): Revert
complexity calculation.
Signed-off-by: Dimitrije Milosevic
---
gcc/tree-ssa-address.cc | 2 +-
gcc/tree-ssa-address.h | 2 +
gcc/tree-ssa-loop-ivopts.cc | 214 ++--
3 files changed, 207 insertions(+), 11 deletions(-)
diff --git a/gcc/tree-ssa-address.cc b/gcc/tree-ssa-address.cc
index ba7b7c93162..442f54f0165 100644
--- a/gcc/tree-ssa-address.cc
+++ b/gcc/tree-ssa-address.cc
@@ -561,7 +561,7 @@ add_to_parts (struct mem_address *parts, tree elt)
validity for a memory reference accessing memory of mode MODE in address
space AS. */
-static bool
+bool
multiplier_allowed_in_address_p (HOST_WIDE_INT ratio, machine_mode mode,
addr_space_t as)
{
diff --git a/gcc/tree-ssa-address.h b/gcc/tree-ssa-address.h
index 95143a099b9..09f36ee2f19 100644
--- a/gcc/tree-ssa-address.h
+++ b/gcc/tree-ssa-address.h
@@ -38,6 +38,8 @@ tree create_mem_ref (gimple_stmt_iterator *, tree,
class aff_tree *, tree, tree, tree, bool);
extern void copy_ref_info (tree, tree);
tree maybe_fold_tmr (tree);
+bool multiplier_allowed_in_address_p (HOST_WIDE_INT ratio, machine_mode mode,
+addr_space_t as);
extern unsigned int preferred_mem_scale_factor (tree base,
machine_mode mem_mode,
diff --git a/gcc/tree-ssa-loop-ivopts.cc b/gcc/tree-ssa-loop-ivopts.cc
index a6f926a68ef..d53ba05a4f6 100644
--- a/gcc/tree-ssa-loop-ivopts.cc
+++ b/gcc/tree-ssa-loop-ivopts.cc
@@ -4774,6 +4774,135 @@ get_address_cost_ainc (poly_int64 ainc_step, poly_int64
ainc_offset,
return infinite_cost;
}
+static void
+compute_symbol_and_var_present (tree e1, tree e2,
+ bool *symbol_present, bool *var_present)
+{
+ poly_uint64_pod off1, off2;
+
+ e1 = strip_offset (e1, );
+ e2 = strip_offset (e2, );
+
+ STRIP_NOPS (e1);
+ STRIP_NOPS (e2);
+
+ if (TREE_CODE (e1) == ADDR_EXPR)
+{
+ poly_int64_pod diff;
+ if (ptr_difference_const (e1, e2, ))
+ {
+*symbol_present = false;
+*var_present = false;
+return;
+ }
+
+ if (integer_zerop (e2))
+ {
+tree core;
+poly_int64_pod bitsize;
+poly_int64_pod bitpos;
+widest_int mul;
+tree toffset;
+machine_mode mode;
+int unsignedp, reversep, volatilep;
+
+core = get_inner_reference (TREE_OPERAND (e1, 0), , ,
+ , , , , );
+
+if (toffset != 0
+|| !constant_multiple_p (bitpos, BITS_PER_UNIT, )
+|| reversep
+|| !VAR_P (core))
+ {
+*symbol_present = false;
+*var_present = true;
+return;
+ }
+
+if (TREE_STATIC (core)
+|| DECL_EXTERNAL (core))
+ {
+*symbol_present = true;
+*var_present = false;
+return;
+ }
+
+*symbol_present = false;
+*var_present = true;
+return;
+ }
+
+ *symbol_present = false;
+ *var_present = true;
+}
+ *symbol_present = false;
+
+ if (operand_equal_p (e1, e2, 0))
+{
+ *var_present = false;
+ return;
+}
+
+ *var_present = true;
+}
+
+static void
+compute_min_and_max_offset (addr_space_t as,
+ machine_mode mem_mode, poly_int64_pod *min_offset,
+ poly_int64_pod *max_offset)
+{
+ machine_mode address_mode = targetm.addr_space.address_mode (as);
+ HOST_WIDE_INT i;
+ poly_int64_pod off, width;
+ rtx addr;
+ rtx reg1;
+
+ reg1 = gen_raw_REG (address_mode, LAST_VIRTUAL_REGISTER + 1);
+
+ width = GET_MODE_BITSIZE (address_mode) - 1;
+ if (known_gt (width, HOST_BITS_PER_WIDE_INT - 1))
+ width = HOST_BITS_PER_WIDE_INT - 1;
+ gcc_assert (width.is_constant ());
+ addr = gen_rtx_fmt_ee (PLUS, address_mode, reg1, NULL_RTX);
+
+ off = 0;
+ for (i = width.to_constant (); i >= 0; i--)
+{
+ off = -(HOST_WIDE_INT_1U << i);
+ XEXP (addr, 1) = gen_int_mode (off, address_mode);
+ if (memory_address_addr_space_p (mem_mode, addr, as))
+break;
+}
+ if (i == -1)
+*min_offset = 0;
+ else
+*min_offset = off;
+ // *min_offset = (i == -1? 0 : off);
+
+ for (i = width.to_constant (); i >= 0; i--)
+{
+ off = (HOST_WIDE_INT_1U << i) - 1;
+ XEXP (addr, 1) = gen_int_mode (off, address_mode);
+ if (memory_address_addr_space
[PATCH 0/2] ivopts: Fix candidate selection for architectures with limited addressing modes.
Architectures like Mips are very limited when it comes to addressing modes. Therefore, the expected behavior would be that, for the BASE + OFFSET addressing mode, complexity is lower, while, for more complex addressing modes (e.g. BASE + INDEX << SCALE), which are not supported, complexity is higher. Currently, the complexity calculation algorithm bails out if BASE + INDEX addressing mode is not supported by the target architecture, resuling in 0-complexities for all candidates, which leads to non-optimal candidate selection, especially in scenarios where there are multiple nested loops. Additionally, when bumping up the register pressure cost, the number of invariants should also be considered, in addition to the number of candidates. Dimitrije Milosevic (2): ivopts: Revert computation of address cost complexity. ivopts: Consider number of invariants when calculating register pressure. gcc/tree-ssa-address.cc | 2 +- gcc/tree-ssa-address.h | 2 + gcc/tree-ssa-loop-ivopts.cc | 220 +--- 3 files changed, 210 insertions(+), 14 deletions(-) --- 2.25.1
Re: [PATCH] Mips: Enable TSAN for 64-bit ABIs
Gentle ping. :)
From: Dimitrije Milosevic
Sent: Friday, July 29, 2022 12:38 PM
To: gcc-patches@gcc.gnu.org
Cc: Djordje Todorovic ; xry...@xry111.site
; mask...@google.com
Subject: [PATCH] Mips: Enable TSAN for 64-bit ABIs
The following patch enables TSAN for mips64, on which it is supported.
Signed-off-by: Dimitrije Milosevic .
libsanitizer/ChangeLog:
* configure.tgt: Enable
TSAN for 64-bit ABIs.
---
libsanitizer/configure.tgt | 4
1 file changed, 4 insertions(+)
diff --git a/libsanitizer/configure.tgt b/libsanitizer/configure.tgt
index fb89df4935c..6855a6ca9e7 100644
--- a/libsanitizer/configure.tgt
+++ b/libsanitizer/configure.tgt
@@ -55,6 +55,10 @@ case "${target}" in
arm*-*-linux*)
;;
mips*-*-linux*)
+ if test x$ac_cv_sizeof_void_p = x8; then
+ TSAN_SUPPORTED=yes
+ TSAN_TARGET_DEPENDENT_OBJECTS=tsan_rtl_mips64.lo
+ fi
;;
aarch64*-*-linux*)
if test x$ac_cv_sizeof_void_p = x8; then
--
2.25.1
[PATCH] Mips: Enable TSAN for 64-bit ABIs
The following patch enables TSAN for mips64, on which it is supported.
Signed-off-by: Dimitrije Milosevic .
libsanitizer/ChangeLog:
* configure.tgt: Enable
TSAN for 64-bit ABIs.
---
libsanitizer/configure.tgt | 4
1 file changed, 4 insertions(+)
diff --git a/libsanitizer/configure.tgt b/libsanitizer/configure.tgt
index fb89df4935c..6855a6ca9e7 100644
--- a/libsanitizer/configure.tgt
+++ b/libsanitizer/configure.tgt
@@ -55,6 +55,10 @@ case "${target}" in
arm*-*-linux*)
;;
mips*-*-linux*)
+ if test x$ac_cv_sizeof_void_p = x8; then
+ TSAN_SUPPORTED=yes
+ TSAN_TARGET_DEPENDENT_OBJECTS=tsan_rtl_mips64.lo
+ fi
;;
aarch64*-*-linux*)
if test x$ac_cv_sizeof_void_p = x8; then
--
2.25.1
Re: [PATCH] libsanitizer: Cherry-pick 2bfb0fcb51510f22723c8cdfefe from upstream
Thanks Martin! I'm sending out the output from git format-patch as an attachment to this email. From: Martin Liška Sent: Thursday, July 28, 2022 3:48 PM To: Dimitrije Milosevic ; gcc-patches@gcc.gnu.org Cc: ma...@embecosm.com ; Djordje Todorovic ; jos...@codesourcery.com Subject: Re: [PATCH] libsanitizer: Cherry-pick 2bfb0fcb51510f22723c8cdfefe from upstream On 7/28/22 15:43, Dimitrije Milosevic wrote: > |Gentle ping, requiring someone to push the change. :)| Sure, I can do that, but please attach output of (git format-patch) as an attachment to an email. The current email has a weird format I can directly apply with git am. Cheers, MartinFrom 9d7646428bf36449cde380230bcc20fa835857dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dimitrije=20Milo=C5=A1evi=C4=87?= Date: Fri, 29 Jul 2022 08:36:06 +0200 Subject: [PATCH] libsanitizer: Cherry-pick 2bfb0fcb51510f22723c8cdfefe from upstream 2bfb0fcb51510f22723c8cdfefe [Sanitizer][MIPS] Fix stat struct size for the O32 ABI. Signed-off-by: Dimitrije Milosevic . --- .../sanitizer_common/sanitizer_platform_limits_posix.h | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h b/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h index 89772a7e5c0..75c6cc7f285 100644 --- a/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h +++ b/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h @@ -81,9 +81,10 @@ const unsigned struct_kernel_stat64_sz = 104; const unsigned struct_kernel_stat_sz = 144; const unsigned struct_kernel_stat64_sz = 104; #elif defined(__mips__) -const unsigned struct_kernel_stat_sz = SANITIZER_ANDROID - ? FIRST_32_SECOND_64(104, 128) - : FIRST_32_SECOND_64(144, 216); +const unsigned struct_kernel_stat_sz = +SANITIZER_ANDROID +? FIRST_32_SECOND_64(104, 128) +: FIRST_32_SECOND_64((_MIPS_SIM == _ABIN32) ? 160 : 144, 216); const unsigned struct_kernel_stat64_sz = 104; #elif defined(__s390__) && !defined(__s390x__) const unsigned struct_kernel_stat_sz = 64; -- 2.25.1
Re: [PATCH] libsanitizer: Cherry-pick 2bfb0fcb51510f22723c8cdfefe from upstream
Gentle ping, requiring someone to push the change. :) From: Dimitrije Milosevic Sent: Monday, July 25, 2022 8:55 AM To: gcc-patches@gcc.gnu.org Cc: Djordje Todorovic ; xry...@xry111.site Subject: [PATCH] libsanitizer: Cherry-pick 2bfb0fcb51510f22723c8cdfefe from upstream 2bfb0fcb51510f22723c8cdfefe [Sanitizer][MIPS] Fix stat struct size for the O32 ABI. Signed-off-by: Dimitrije Milosevic . --- .../sanitizer_common/sanitizer_platform_limits_posix.h | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h b/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h index 89772a7e5c0..75c6cc7f285 100644 --- a/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h +++ b/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h @@ -81,9 +81,10 @@ const unsigned struct_kernel_stat64_sz = 104; const unsigned struct_kernel_stat_sz = 144; const unsigned struct_kernel_stat64_sz = 104; #elif defined(__mips__) -const unsigned struct_kernel_stat_sz = SANITIZER_ANDROID - ? FIRST_32_SECOND_64(104, 128) - : FIRST_32_SECOND_64(144, 216); +const unsigned struct_kernel_stat_sz = + SANITIZER_ANDROID + ? FIRST_32_SECOND_64(104, 128) + : FIRST_32_SECOND_64((_MIPS_SIM == _ABIN32) ? 160 : 144, 216); const unsigned struct_kernel_stat64_sz = 104; #elif defined(__s390__) && !defined(__s390x__) const unsigned struct_kernel_stat_sz = 64; -- 2.25.1
Re: PING: [PATCH] libsanitizer: Cherry-pick 2bfb0fcb51510f22723c8cdfefe from upstream
> Do you know someone very familiar with MIPS and GCC and capable as a > port maintainer? An active MIPS port maintainer will make the situation > better. Sadly, no. I agree it would make things easier. From: Xi Ruoyao Sent: Wednesday, July 27, 2022 8:43 AM To: Dimitrije Milosevic ; gcc-patches@gcc.gnu.org Cc: Djordje Todorovic ; richard.sandif...@arm.com Subject: Re: PING: [PATCH] libsanitizer: Cherry-pick 2bfb0fcb51510f22723c8cdfefe from upstream On Wed, 2022-07-27 at 06:41 +, Dimitrije Milosevic wrote: > Gentle ping, requiring someone to push this change, as I do not have > commit access. :) Do you know someone very familiar with MIPS and GCC and capable as a port maintainer? An active MIPS port maintainer will make the situation better. -- Xi Ruoyao School of Aerospace Science and Technology, Xidian University
PING: [PATCH] libsanitizer: Cherry-pick 2bfb0fcb51510f22723c8cdfefe from upstream
Gentle ping, requiring someone to push this change, as I do not have commit access. :) From: Dimitrije Milosevic Sent: Monday, July 25, 2022 8:55 AM To: gcc-patches@gcc.gnu.org Cc: Djordje Todorovic ; xry...@xry111.site Subject: [PATCH] libsanitizer: Cherry-pick 2bfb0fcb51510f22723c8cdfefe from upstream 2bfb0fcb51510f22723c8cdfefe [Sanitizer][MIPS] Fix stat struct size for the O32 ABI. Signed-off-by: Dimitrije Milosevic . --- .../sanitizer_common/sanitizer_platform_limits_posix.h | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h b/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h index 89772a7e5c0..75c6cc7f285 100644 --- a/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h +++ b/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h @@ -81,9 +81,10 @@ const unsigned struct_kernel_stat64_sz = 104; const unsigned struct_kernel_stat_sz = 144; const unsigned struct_kernel_stat64_sz = 104; #elif defined(__mips__) -const unsigned struct_kernel_stat_sz = SANITIZER_ANDROID - ? FIRST_32_SECOND_64(104, 128) - : FIRST_32_SECOND_64(144, 216); +const unsigned struct_kernel_stat_sz = + SANITIZER_ANDROID + ? FIRST_32_SECOND_64(104, 128) + : FIRST_32_SECOND_64((_MIPS_SIM == _ABIN32) ? 160 : 144, 216); const unsigned struct_kernel_stat64_sz = 104; #elif defined(__s390__) && !defined(__s390x__) const unsigned struct_kernel_stat_sz = 64; -- 2.25.1
[PATCH] libsanitizer: Cherry-pick 2bfb0fcb51510f22723c8cdfefe from upstream
2bfb0fcb51510f22723c8cdfefe [Sanitizer][MIPS] Fix stat struct size for the O32 ABI. Signed-off-by: Dimitrije Milosevic . --- .../sanitizer_common/sanitizer_platform_limits_posix.h | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h b/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h index 89772a7e5c0..75c6cc7f285 100644 --- a/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h +++ b/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h @@ -81,9 +81,10 @@ const unsigned struct_kernel_stat64_sz = 104; const unsigned struct_kernel_stat_sz = 144; const unsigned struct_kernel_stat64_sz = 104; #elif defined(__mips__) -const unsigned struct_kernel_stat_sz = SANITIZER_ANDROID - ? FIRST_32_SECOND_64(104, 128) - : FIRST_32_SECOND_64(144, 216); +const unsigned struct_kernel_stat_sz = +SANITIZER_ANDROID +? FIRST_32_SECOND_64(104, 128) +: FIRST_32_SECOND_64((_MIPS_SIM == _ABIN32) ? 160 : 144, 216); const unsigned struct_kernel_stat64_sz = 104; #elif defined(__s390__) && !defined(__s390x__) const unsigned struct_kernel_stat_sz = 64; -- 2.25.1
Re: Mips: Fix kernel_stat structure size
Hi Xi, > Please CC me (@xry111) in LLVM review. I just posted the patch. You should've gotten the email. If not, see: https://reviews.llvm.org/D129749. > By the way, if you have some spare time, you can also add the value for > Musl (all 3 ABIs). Musl has a different struct stat than Glibc (it's PR > 106136 in GCC bugzilla). I might be able to, but no promises. :) From: Xi Ruoyao Sent: Wednesday, July 13, 2022 4:38 AM To: Dimitrije Milosevic ; Hans-Peter Nilsson Cc: Djordje Todorovic ; gcc-patches@gcc.gnu.org Subject: Re: Mips: Fix kernel_stat structure size On Tue, 2022-07-12 at 06:42 +0000, Dimitrije Milosevic wrote: > I will send out a review, covering both 32 ABIs, meaning that both O32 > and N32 ABIs will be working properly. Please CC me (@xry111) in LLVM review. By the way, if you have some spare time, you can also add the value for Musl (all 3 ABIs). Musl has a different struct stat than Glibc (it's PR 106136 in GCC bugzilla). -- Xi Ruoyao School of Aerospace Science and Technology, Xidian University
Re: Mips: Fix kernel_stat structure size
Hi Hans-Peter, You're right, this is not ok for the O32 ABI. Your change however, broke the functionality for the N32 ABI. AFAIK, the changes like this should go through LLVM first (yours didn't), so I will send out a review, covering both 32 ABIs, meaning that both O32 and N32 ABIs will be working properly. As for this change, I'm not sure what should be done? Should this be committed now, while the LLVM change is cherry-picked once it's committed. Best regards, Dimitrije Milosevic From: Hans-Peter Nilsson Sent: Saturday, July 9, 2022 4:44 PM To: Xi Ruoyao Cc: Dimitrije Milosevic ; Djordje Todorovic ; gcc-patches@gcc.gnu.org Subject: Re: Mips: Fix kernel_stat structure size On Sat, 9 Jul 2022, Xi Ruoyao wrote: > On Fri, 2022-07-08 at 21:42 -0400, Hans-Peter Nilsson wrote: > > On Fri, 1 Jul 2022, Dimitrije Milosevic wrote: > > > > > Fix kernel_stat structure size for non-Android 32-bit Mips. > > > LLVM currently has this value for the kernel_stat structure size, > > > as per compiler-rt/lib/sanitizer- > > > common/sanitizer_platform_limits_posix.h. > > > This also resolves one of the build issues for non-Android 32-bit > > > Mips. > > > > I insist that PR105614 comment #7 is the way to go, i.e. fix > > the merge error, avoiding the faulty include that it > > reintroduced.? Was this tested on O32? > > I'm pretty sure it is *not* the way to go. > > Sanitizer does not really intercept system call. It intercepts libc > stat() or lstat() etc. calls. So you need to keep struct_kernel_stat_sz > same as the size of struct stat in libc, i. e. "the size of buffer which > *libc* stat()-like functions writing into". > > The "kernel_" in the name is just misleading. You make a sound argument, and I just refer to my old commit 9f943b2446f2d0a. Either way, the proof is in the pussing: was this tested for O32 or not? > And, if you still think it should be the way to go, let's submit the > change to LLVM and get it reviewed properly. Sorry, I've no longer interest in mips besides I'd like to see un-broken what I helped getting in a working state (support ASAN in gcc for mips O32). brgds, H-P
Re: [PATCH] Mips: Resolve build issues for the n32 ABI
Ping. :)
This change just landed on LLVM (see
https://reviews.llvm.org/rG5d8077565e4196efdd4ed525a64c11a96d5aa5dd).
Unfortunately, I do not have commit access, so if anyone can commit it, that
would be great!
Here is the patch with the updated commit message.
libsanitizer: Cherry-pick 5d8077565e41 from upstream
Building the ASAN for the n32 MIPS ABI currently fails, due to a few reasons:
- defined(__mips64), which is set solely based on the architecture type
(32-bit/64-bit),
was still used in some places. Therefore, defined(__mips64) is swapped with
SANITIZER_MIPS64,
which takes the ABI into account as well - defined(__mips64) &&
_MIPS_SIM == ABI64.
- The n32 ABI still uses 64-bit *Linux* system calls, even though the word size
is 32 bits.
- After the transition to canonical system calls
(https://reviews.llvm.org/D124212), the n32 ABI still didn't use them,
even though they are supported,
as per
https://github.com/torvalds/linux/blob/master/arch/mips/kernel/syscalls/syscall_n32.tbl.
See https://reviews.llvm.org/D127098.
libsanitizer/ChangeLog:
* sanitizer_common/sanitizer_linux.cpp (defined): Resolve
ASAN build issues for the Mips n32 ABI.
* sanitizer_common/sanitizer_platform.h (defined): Likewise.
---
.../sanitizer_common/sanitizer_linux.cpp| 17 ++---
.../sanitizer_common/sanitizer_platform.h | 2 +-
2 files changed, 11 insertions(+), 8 deletions(-)
diff --git a/libsanitizer/sanitizer_common/sanitizer_linux.cpp
b/libsanitizer/sanitizer_common/sanitizer_linux.cpp
index e2c32d679ad..5ba033492e7 100644
--- a/libsanitizer/sanitizer_common/sanitizer_linux.cpp
+++ b/libsanitizer/sanitizer_common/sanitizer_linux.cpp
@@ -34,7 +34,7 @@
// format. Struct kernel_stat is defined as 'struct stat' in asm/stat.h. To
// access stat from asm/stat.h, without conflicting with definition in
// sys/stat.h, we use this trick.
-#if defined(__mips64)
+#if SANITIZER_MIPS64
#include
#include
#define stat kernel_stat
@@ -124,8 +124,9 @@ const int FUTEX_WAKE_PRIVATE = FUTEX_WAKE |
FUTEX_PRIVATE_FLAG;
// Are we using 32-bit or 64-bit Linux syscalls?
// x32 (which defines __x86_64__) has SANITIZER_WORDSIZE == 32
// but it still needs to use 64-bit syscalls.
-#if SANITIZER_LINUX && (defined(__x86_64__) || defined(__powerpc64__) ||
\
-SANITIZER_WORDSIZE == 64)
+#if SANITIZER_LINUX && (defined(__x86_64__) || defined(__powerpc64__) || \
+SANITIZER_WORDSIZE == 64 || \
+(defined(__mips__) && _MIPS_SIM == _ABIN32))
# define SANITIZER_LINUX_USES_64BIT_SYSCALLS 1
#else
# define SANITIZER_LINUX_USES_64BIT_SYSCALLS 0
@@ -289,7 +290,7 @@ static void stat64_to_stat(struct stat64 *in, struct stat
*out) {
}
#endif
-#if defined(__mips64)
+#if SANITIZER_MIPS64
// Undefine compatibility macros from
// so that they would not clash with the kernel_stat
// st_[a|m|c]time fields
@@ -343,7 +344,8 @@ uptr internal_stat(const char *path, void *buf) {
#if SANITIZER_FREEBSD
return internal_syscall(SYSCALL(fstatat), AT_FDCWD, (uptr)path, (uptr)buf,
0);
#elif SANITIZER_LINUX
-# if SANITIZER_WORDSIZE == 64 || SANITIZER_X32
+# if SANITIZER_WORDSIZE == 64 || SANITIZER_X32 || \
+ (defined(__mips__) && _MIPS_SIM == _ABIN32)
return internal_syscall(SYSCALL(newfstatat), AT_FDCWD, (uptr)path, (uptr)buf,
0);
# else
@@ -366,7 +368,8 @@ uptr internal_lstat(const char *path, void *buf) {
return internal_syscall(SYSCALL(fstatat), AT_FDCWD, (uptr)path, (uptr)buf,
AT_SYMLINK_NOFOLLOW);
#elif SANITIZER_LINUX
-# if defined(_LP64) || SANITIZER_X32
+# if defined(_LP64) || SANITIZER_X32 || \
+ (defined(__mips__) && _MIPS_SIM == _ABIN32)
return internal_syscall(SYSCALL(newfstatat), AT_FDCWD, (uptr)path, (uptr)buf,
AT_SYMLINK_NOFOLLOW);
# else
@@ -1053,7 +1056,7 @@ uptr GetMaxVirtualAddress() {
return (1ULL << (MostSignificantSetBitIndex(GET_CURRENT_FRAME()) + 1)) - 1;
#elif SANITIZER_RISCV64
return (1ULL << 38) - 1;
-# elif defined(__mips64)
+# elif SANITIZER_MIPS64
return (1ULL << 40) - 1; // 0x00ffUL;
# elif defined(__s390x__)
return (1ULL << 53) - 1; // 0x001fUL;
diff --git a/libsanitizer/sanitizer_common/sanitizer_platform.h
b/libsanitizer/sanitizer_common/sanitizer_platform.h
index 8fe0d831431..8bd9a327623 100644
--- a/libsanitizer/sanitizer_common/sanitizer_platform.h
+++ b/libsanitizer/sanitizer_common/sanitizer_platform.h
@@ -159,7 +159,7 @@
#if defined(__mips__)
# define SANITIZER_MIPS 1
-# if defined(__mips64)
+# if defined(__mips64) && _MIPS_SIM == _ABI64
#define SANITIZER_MIPS32 0
#define SANITIZER_MIPS64 1
# else
--
2.25.1
From: Richard Sandiford
Sent: Monday, July 4, 2022 1:23
Re: Mips: Fix kernel_stat structure size
Ping. :) I think this is good to go. Unfortunately, I do not have commit access, so if anyone can commit it, that would be great! From: Dimitrije Milosevic Sent: Friday, July 1, 2022 4:25 PM To: Xi Ruoyao ; gcc-patches@gcc.gnu.org Cc: Djordje Todorovic ; Richard Sandiford Subject: Re: Mips: Fix kernel_stat structure size Thanks Xi. Forgive me as I'm not that familiar with the coding standards when submitting patches for a review. Here is the updated version of the patch. Fix kernel_stat structure size for non-Android 32-bit Mips. LLVM currently has this value for the kernel_stat structure size, as per compiler-rt/lib/sanitizer-common/sanitizer_platform_limits_posix.h. This also resolves one of the build issues for non-Android 32-bit Mips. libsanitizer/ChangeLog: * sanitizer_common/sanitizer_platform_limits_posix.h: Fix kernel_stat structure size for non-Android 32-bit Mips. --- libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h b/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h index 89772a7e5c0..62a99035db3 100644 --- a/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h +++ b/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h @@ -83,7 +83,7 @@ const unsigned struct_kernel_stat64_sz = 104; #elif defined(__mips__) const unsigned struct_kernel_stat_sz = SANITIZER_ANDROID ? FIRST_32_SECOND_64(104, 128) - : FIRST_32_SECOND_64(144, 216); + : FIRST_32_SECOND_64(160, 216); const unsigned struct_kernel_stat64_sz = 104; #elif defined(__s390__) && !defined(__s390x__) const unsigned struct_kernel_stat_sz = 64; ---
Re: [PATCH] Mips: Enable asynchronous unwind tables with both ASAN and TSAN
Hi Xi.
Correct, I am using a simulator.
Here are my changes:
diff --git a/libsanitizer/configure.tgt b/libsanitizer/configure.tgt
index fb89df4935c..6855a6ca9e7 100644
--- a/libsanitizer/configure.tgt
+++ b/libsanitizer/configure.tgt
@@ -55,6 +55,10 @@ case "${target}" in
arm*-*-linux*)
;;
mips*-*-linux*)
+ if test x$ac_cv_sizeof_void_p = x8; then
+ TSAN_SUPPORTED=yes
+ TSAN_TARGET_DEPENDENT_OBJECTS=tsan_rtl_mips64.lo
+ fi
;;
aarch64*-*-linux*)
if test x$ac_cv_sizeof_void_p = x8; then
Nit: Updated the code in gcc/config/mips/mips.cc a little bit, but no functional
change compared to your version.
diff --git a/gcc/config/mips/mips.cc b/gcc/config/mips/mips.cc
index 8a55d2fb8f7..4ccc9e75482 100644
--- a/gcc/config/mips/mips.cc
+++ b/gcc/config/mips/mips.cc
@@ -20118,13 +20118,12 @@ mips_option_override (void)
/* -fsanitize=address needs to turn on -fasynchronous-unwind-tables
in order for tracebacks to be complete but not if any
-fasynchronous-unwind-table were already specified. */
- /* FIXME: TSAN could also utilize -fasynchronous-unwind-tables, but
- should be first enabled for MIPS64.
- FIXME: HWSAN could also utilize -fasynchronous-unwind-tables, but,
+ /* FIXME: HWSAN could also utilize -fasynchronous-unwind-tables, but,
currently, it is only available on AArch64.
FIXME: We would also like to check if -ffreestanding is passed in.
However, it is only available in C-ish frontends. */
- if ((flag_sanitize & SANITIZE_USER_ADDRESS)
+ if (((flag_sanitize & SANITIZE_USER_ADDRESS)
+ || (TARGET_64BIT && (flag_sanitize & SANITIZE_THREAD)))
&& !global_options_set.x_flag_asynchronous_unwind_tables)
flag_asynchronous_unwind_tables = 1;
From: Xi Ruoyao
Sent: Tuesday, July 5, 2022 3:54 AM
To: Dimitrije Milosevic ;
gcc-patches@gcc.gnu.org
Cc: Djordje Todorovic
Subject: Re: [PATCH] Mips: Enable asynchronous unwind tables with both ASAN and
TSAN
On Mon, 2022-07-04 at 14:28 +, Dimitrije Milosevic wrote:
> On Saturday, June 11, 2022 2:03 PM, Xi wrote:
> > Just tried TSAN_SUPPORTED=yes with asynchronous unwind tables
> > enabled,
> > but I got some strange test failures for tls_race.c:
> >
> > FAIL: c-c++-common/tsan/tls_race.c -O0 output pattern test
> > Output was:
> > ThreadSanitizer: CHECK failed: tsan_platform_linux.cpp:452
> > "((thr_end)) <= ((tls_addr + tls_size))" (0xffec35f8c0,
> > 0xffec35f784) (tid=748216)
> > #0 __tsan::CheckUnwind()
> > ../../../../gcc/libsanitizer/tsan/tsan_rtl.cpp:627
> > (libtsan.so.2+0xa30ec)
> > #1 __sanitizer::CheckFailed(char const*, int, char const*,
> > unsigned long long, unsigned long long)
> > ../../../../gcc/libsanitizer/sanitizer_common/sanitizer_termination.
> > cpp:86 (libtsan.so.2+0xeb8cc)
> > #2 __tsan::ImitateTlsWrite(__tsan::ThreadState*, unsigned long,
> > unsigned long)
> > ../../../../gcc/libsanitizer/tsan/tsan_platform_linux.cpp:452
> > (libtsan.so.2+0xa0cac)
> > #3 __tsan::ThreadStart(__tsan::ThreadState*, unsigned int,
> > unsigned long long, __sanitizer::ThreadType)
> > ../../../../gcc/libsanitizer/tsan/tsan_rtl_thread.cpp:197
> > (libtsan.so.2+0xc0e88)
> > #4 __tsan_thread_start_func
> > ../../../../gcc/libsanitizer/tsan/tsan_interceptors_posix.cpp:1009
> > (libtsan.so.2+0x3e5dc)
> > #5 start_thread /sources/glibc-2.35/nptl/pthread_create.c:442
> > (libc.so.6+0xc75f4)
> >
> > I've tried to diagnose the root cause but failed.
>
> Hi Xi, thanks for looking into this. I've tried running the testsuite
> on a cross-toolchain (as I do not currently have access to a physical
> machine)
> for a MIPS64R6 and the test passes successfully. Could you please
> verify that the test fails solely based on this change?
I guess you mean you are running MIPS64R6 target code with qemu? I'm
not 100% sure because maybe something is wrong in my system. I'm now
retrying on gcc230.fsffrance.org (an EdgeRouter 4 in Cfarm) but building
GCC on it is really slow.
The changes I've tested:
diff --git a/gcc/config/mips/mips.cc b/gcc/config/mips/mips.cc
index 5eb845960e1..a7f0580e9ba 100644
--- a/gcc/config/mips/mips.cc
+++ b/gcc/config/mips/mips.cc
@@ -20115,10 +20115,11 @@ mips_option_override (void)
target_flags |= MASK_64BIT;
}
- /* -fsanitize=address needs to turn on -fasynchronous-unwind-tables in
- order for tracebacks to be complete but not if any
- -fasynchronous-unwind-table were already specified. */
- if (flag_sanitize & SANITIZE_USER_ADDRESS
+ /* For -fsanitize=address or -fsanitize=thread, it's needed to turn
+ on -fasynchronous-unwind-tables in order for tracebacks
+ to be complete but not if any
Re: [PATCH] Mips: Enable asynchronous unwind tables with both ASAN and TSAN
On Saturday, June 11, 2022 2:03 PM, Xi wrote: > Just tried TSAN_SUPPORTED=yes with asynchronous unwind tables enabled, > but I got some strange test failures for tls_race.c: > > FAIL: c-c++-common/tsan/tls_race.c -O0 output pattern test > Output was: > ThreadSanitizer: CHECK failed: tsan_platform_linux.cpp:452 "((thr_end)) <= > ((tls_addr + tls_size))" (0xffec35f8c0, 0xffec35f784) (tid=748216) > #0 __tsan::CheckUnwind() > ../../../../gcc/libsanitizer/tsan/tsan_rtl.cpp:627 (libtsan.so.2+0xa30ec) > #1 __sanitizer::CheckFailed(char const*, int, char const*, unsigned long > long, unsigned long long) > ../../../../gcc/libsanitizer/sanitizer_common/sanitizer_termination.cpp:86 > (libtsan.so.2+0xeb8cc) > #2 __tsan::ImitateTlsWrite(__tsan::ThreadState*, unsigned long, unsigned > long) ../../../../gcc/libsanitizer/tsan/tsan_platform_linux.cpp:452 > (libtsan.so.2+0xa0cac) > #3 __tsan::ThreadStart(__tsan::ThreadState*, unsigned int, unsigned long > long, __sanitizer::ThreadType) > ../../../../gcc/libsanitizer/tsan/tsan_rtl_thread.cpp:197 > (libtsan.so.2+0xc0e88) > #4 __tsan_thread_start_func > ../../../../gcc/libsanitizer/tsan/tsan_interceptors_posix.cpp:1009 > (libtsan.so.2+0x3e5dc) > #5 start_thread /sources/glibc-2.35/nptl/pthread_create.c:442 > (libc.so.6+0xc75f4) > > I've tried to diagnose the root cause but failed. Hi Xi, thanks for looking into this. I've tried running the testsuite on a cross-toolchain (as I do not currently have access to a physical machine) for a MIPS64R6 and the test passes successfully. Could you please verify that the test fails solely based on this change? Kind regards, Dimitrije
Re: [PATCH] Mips: Resolve build issues for the n32 ABI
Thanks Xi. Forgive me as I'm not that familiar with the coding standards
when submitting patches for a review.
Here is the updated version of the patch.
Building the ASAN for the n32 MIPS ABI currently fails, due to a few reasons:
- defined(__mips64), which is set solely based on the architecture type
(32-bit/64-bit),
was still used in some places. Therefore, defined(__mips64) is swapped with
SANITIZER_MIPS64,
which takes the ABI into account as well - defined(__mips64) &&
_MIPS_SIM == ABI64.
- The n32 ABI still uses 64-bit *Linux* system calls, even though the word size
is 32 bits.
- After the transition to canonical system calls
(https://reviews.llvm.org/D124212), the n32 ABI still didn't use them,
even though they are supported,
as per
https://github.com/torvalds/linux/blob/master/arch/mips/kernel/syscalls/syscall_n32.tbl.
See https://reviews.llvm.org/D127098.
libsanitizer/ChangeLog:
* sanitizer_common/sanitizer_linux.cpp (defined): Resolve
ASAN build issues for the Mips n32 ABI.
* sanitizer_common/sanitizer_platform.h (defined): Likewise.
---
libsanitizer/sanitizer_common/sanitizer_linux.cpp | 17 ++---
libsanitizer/sanitizer_common/sanitizer_platform.h | 2 +-
2 files changed, 11 insertions(+), 8 deletions(-)
diff --git a/libsanitizer/sanitizer_common/sanitizer_linux.cpp
b/libsanitizer/sanitizer_common/sanitizer_linux.cpp
index e2c32d679ad..5ba033492e7 100644
--- a/libsanitizer/sanitizer_common/sanitizer_linux.cpp
+++ b/libsanitizer/sanitizer_common/sanitizer_linux.cpp
@@ -34,7 +34,7 @@
// format. Struct kernel_stat is defined as 'struct stat' in asm/stat.h. To
// access stat from asm/stat.h, without conflicting with definition in
// sys/stat.h, we use this trick.
-#if defined(__mips64)
+#if SANITIZER_MIPS64
#include
#include
#define stat kernel_stat
@@ -124,8 +124,9 @@ const int FUTEX_WAKE_PRIVATE = FUTEX_WAKE |
FUTEX_PRIVATE_FLAG;
// Are we using 32-bit or 64-bit Linux syscalls?
// x32 (which defines __x86_64__) has SANITIZER_WORDSIZE == 32
// but it still needs to use 64-bit syscalls.
-#if SANITIZER_LINUX && (defined(__x86_64__) || defined(__powerpc64__) ||
\
-SANITIZER_WORDSIZE == 64)
+#if SANITIZER_LINUX && (defined(__x86_64__) || defined(__powerpc64__) || \
+SANITIZER_WORDSIZE == 64 || \
+(defined(__mips__) && _MIPS_SIM == _ABIN32))
# define SANITIZER_LINUX_USES_64BIT_SYSCALLS 1
#else
# define SANITIZER_LINUX_USES_64BIT_SYSCALLS 0
@@ -289,7 +290,7 @@ static void stat64_to_stat(struct stat64 *in, struct stat
*out) {
}
#endif
-#if defined(__mips64)
+#if SANITIZER_MIPS64
// Undefine compatibility macros from
// so that they would not clash with the kernel_stat
// st_[a|m|c]time fields
@@ -343,7 +344,8 @@ uptr internal_stat(const char *path, void *buf) {
#if SANITIZER_FREEBSD
return internal_syscall(SYSCALL(fstatat), AT_FDCWD, (uptr)path, (uptr)buf,
0);
#elif SANITIZER_LINUX
-# if SANITIZER_WORDSIZE == 64 || SANITIZER_X32
+# if SANITIZER_WORDSIZE == 64 || SANITIZER_X32 || \
+ (defined(__mips__) && _MIPS_SIM == _ABIN32)
return internal_syscall(SYSCALL(newfstatat), AT_FDCWD, (uptr)path, (uptr)buf,
0);
# else
@@ -366,7 +368,8 @@ uptr internal_lstat(const char *path, void *buf) {
return internal_syscall(SYSCALL(fstatat), AT_FDCWD, (uptr)path, (uptr)buf,
AT_SYMLINK_NOFOLLOW);
#elif SANITIZER_LINUX
-# if defined(_LP64) || SANITIZER_X32
+# if defined(_LP64) || SANITIZER_X32 || \
+ (defined(__mips__) && _MIPS_SIM == _ABIN32)
return internal_syscall(SYSCALL(newfstatat), AT_FDCWD, (uptr)path, (uptr)buf,
AT_SYMLINK_NOFOLLOW);
# else
@@ -1053,7 +1056,7 @@ uptr GetMaxVirtualAddress() {
return (1ULL << (MostSignificantSetBitIndex(GET_CURRENT_FRAME()) + 1)) - 1;
#elif SANITIZER_RISCV64
return (1ULL << 38) - 1;
-# elif defined(__mips64)
+# elif SANITIZER_MIPS64
return (1ULL << 40) - 1; // 0x00ffUL;
# elif defined(__s390x__)
return (1ULL << 53) - 1; // 0x001fUL;
diff --git a/libsanitizer/sanitizer_common/sanitizer_platform.h
b/libsanitizer/sanitizer_common/sanitizer_platform.h
index 8fe0d831431..8bd9a327623 100644
--- a/libsanitizer/sanitizer_common/sanitizer_platform.h
+++ b/libsanitizer/sanitizer_common/sanitizer_platform.h
@@ -159,7 +159,7 @@
#if defined(__mips__)
# define SANITIZER_MIPS 1
-# if defined(__mips64)
+# if defined(__mips64) && _MIPS_SIM == _ABI64
#define SANITIZER_MIPS32 0
#define SANITIZER_MIPS64 1
# else
---
Re: Mips: Fix kernel_stat structure size
Thanks Xi. Forgive me as I'm not that familiar with the coding standards when submitting patches for a review. Here is the updated version of the patch. Fix kernel_stat structure size for non-Android 32-bit Mips. LLVM currently has this value for the kernel_stat structure size, as per compiler-rt/lib/sanitizer-common/sanitizer_platform_limits_posix.h. This also resolves one of the build issues for non-Android 32-bit Mips. libsanitizer/ChangeLog: * sanitizer_common/sanitizer_platform_limits_posix.h: Fix kernel_stat structure size for non-Android 32-bit Mips. --- libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h b/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h index 89772a7e5c0..62a99035db3 100644 --- a/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h +++ b/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h @@ -83,7 +83,7 @@ const unsigned struct_kernel_stat64_sz = 104; #elif defined(__mips__) const unsigned struct_kernel_stat_sz = SANITIZER_ANDROID ? FIRST_32_SECOND_64(104, 128) - : FIRST_32_SECOND_64(144, 216); + : FIRST_32_SECOND_64(160, 216); const unsigned struct_kernel_stat64_sz = 104; #elif defined(__s390__) && !defined(__s390x__) const unsigned struct_kernel_stat_sz = 64; ---
[PATCH] libsanitizer: Fix linkage errors for cross toolchains
When we use cross toolchains, in which the GCC libraries are not installed
within a designated system root, the shared sanitizer libraries link against
libstdc++.so* within the same libraries. This directory, however, is not in
RPATH,
so attempting to build a dynamically linked application with -fsanitize=...
gives a linkage error.
More information can be found here:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69839.
GCC, even when configured with -with-sysroot, by default, doesn't install
libstdc++.so*
within the sysroot, as GCC's installation process isn't designed to help
construct sysroot trees.
This has to be done manually.
Furthermore, if we are using a multiarch/multilib configuration (mips-mti*, for
example),
we may not even want to install them within the sysroot.
Would love to hear your thoughts on this, as I'm not sure myself that this is
the best solution.
gcc/ChangeLog:
* gcc.cc (LIBSAN_RPATH): New macro.
(LIBASAN_SPEC): Add LIBSAN_RPATH.
(LIBTSAN_SPEC): Likewise.
(LIBLSAN_SPEC): Likewise.
(LIBUBSAN_SPEC): Likewise.
libsanitizer/ChangeLog:
* Makefile.in: New Makefile variable.
* asan/Makefile.in: Likewise.
* configure: Regenerate.
* configure.ac: New config variable.
* hwasan/Makefile.in: New Makefile variable.
* interception/Makefile.in: Likewise.
* libbacktrace/Makefile.in: Likewise.
* libsanitizer.spec.in: New spec.
* lsan/Makefile.in: New Makefile variable.
* sanitizer_common/Makefile.in: Likewise.
* tsan/Makefile.in: Likewise.
* ubsan/Makefile.in: Likewise.
---
gcc/gcc.cc| 20
libsanitizer/Makefile.in | 1 +
libsanitizer/asan/Makefile.in | 1 +
libsanitizer/configure| 10 --
libsanitizer/configure.ac | 7 +++
libsanitizer/hwasan/Makefile.in | 1 +
libsanitizer/interception/Makefile.in | 1 +
libsanitizer/libbacktrace/Makefile.in | 1 +
libsanitizer/libsanitizer.spec.in | 2 ++
libsanitizer/lsan/Makefile.in | 1 +
libsanitizer/sanitizer_common/Makefile.in | 1 +
libsanitizer/tsan/Makefile.in | 1 +
libsanitizer/ubsan/Makefile.in| 1 +
13 files changed, 38 insertions(+), 10 deletions(-)
diff --git a/gcc/gcc.cc b/gcc/gcc.cc
index 299e09c4f54..0d2d361b9a4 100644
--- a/gcc/gcc.cc
+++ b/gcc/gcc.cc
@@ -738,17 +738,21 @@ proper position among the other output files. */
#define STACK_SPLIT_SPEC " %{fsplit-stack: --wrap=pthread_create}"
#endif
+#ifndef LIBSAN_RPATH
+#define LIBSAN_RPATH " %:include(libsanitizer.spec)%(link_libsan_rpath)"
+#endif
+
#ifndef LIBASAN_SPEC
#define STATIC_LIBASAN_LIBS \
" %{static-libasan|static:%:include(libsanitizer.spec)%(link_libasan)}"
#ifdef LIBASAN_EARLY_SPEC
-#define LIBASAN_SPEC STATIC_LIBASAN_LIBS
+#define LIBASAN_SPEC STATIC_LIBASAN_LIBS LIBSAN_RPATH
#elif defined(HAVE_LD_STATIC_DYNAMIC)
#define LIBASAN_SPEC "%{static-libasan:" LD_STATIC_OPTION \
"} -lasan %{static-libasan:" LD_DYNAMIC_OPTION "}" \
STATIC_LIBASAN_LIBS
#else
-#define LIBASAN_SPEC "-lasan" STATIC_LIBASAN_LIBS
+#define LIBASAN_SPEC "-lasan" STATIC_LIBASAN_LIBS LIBSAN_RPATH
#endif
#endif
@@ -778,13 +782,13 @@ proper position among the other output files. */
#define STATIC_LIBTSAN_LIBS \
" %{static-libtsan|static:%:include(libsanitizer.spec)%(link_libtsan)}"
#ifdef LIBTSAN_EARLY_SPEC
-#define LIBTSAN_SPEC STATIC_LIBTSAN_LIBS
+#define LIBTSAN_SPEC STATIC_LIBTSAN_LIBS LIBSAN_RPATH
#elif defined(HAVE_LD_STATIC_DYNAMIC)
#define LIBTSAN_SPEC "%{static-libtsan:" LD_STATIC_OPTION \
"} -ltsan %{static-libtsan:" LD_DYNAMIC_OPTION "}" \
STATIC_LIBTSAN_LIBS
#else
-#define LIBTSAN_SPEC "-ltsan" STATIC_LIBTSAN_LIBS
+#define LIBTSAN_SPEC "-ltsan" STATIC_LIBTSAN_LIBS LIBSAN_RPATH
#endif
#endif
@@ -796,13 +800,13 @@ proper position among the other output files. */
#define STATIC_LIBLSAN_LIBS \
" %{static-liblsan|static:%:include(libsanitizer.spec)%(link_liblsan)}"
#ifdef LIBLSAN_EARLY_SPEC
-#define LIBLSAN_SPEC STATIC_LIBLSAN_LIBS
+#define LIBLSAN_SPEC STATIC_LIBLSAN_LIBS LIBSAN_RPATH
#elif defined(HAVE_LD_STATIC_DYNAMIC)
#define LIBLSAN_SPEC "%{static-liblsan:" LD_STATIC_OPTION \
"} -llsan %{static-liblsan:" LD_DYNAMIC_OPTION "}" \
STATIC_LIBLSAN_LIBS
#else
-#define LIBLSAN_SPEC "-llsan" STATIC_LIBLSAN_LIBS
+#define LIBLSAN_SPEC "-llsan" STATIC_LIBLSAN_LIBS LIBSAN_RPATH
#endif
#endif
@@ -816,9 +820,9 @@ proper position among the other output files. */
#ifdef HAVE_LD_STATIC_DYNAMIC
#define LIBUBSAN_SPEC "%{static-libubsan:" LD_STATIC_OPTION \
"} -lubsan %{static-libubsan:"
[PATCH] Mips: Resolve build issues for the n32 ABI
Building the ASAN for the n32 MIPS ABI currently fails, due to a few reasons:
- defined(__mips64), which is set solely based on the architecture type
(32-bit/64-bit),
was still used in some places. Therefore, defined(__mips64) is swapped with
SANITIZER_MIPS64,
which takes the ABI into account as well - defined(__mips64) &&
_MIPS_SIM == ABI64.
- The n32 ABI still uses 64-bit *Linux* system calls, even though the word size
is 32 bits.
- After the transition to canonical system calls
(https://reviews.llvm.org/D124212), the n32 ABI still didn't use them,
even though they are supported,
as per
https://github.com/torvalds/linux/blob/master/arch/mips/kernel/syscalls/syscall_n32.tbl.
See https://reviews.llvm.org/D127098.
libsanitizer/ChangeLog:
* sanitizer_common/sanitizer_linux.cpp (defined): Resolve
ASAN build issues for the Mips n32 ABI.
* sanitizer_common/sanitizer_platform.h (defined): Likewise.
---
libsanitizer/sanitizer_common/sanitizer_linux.cpp | 17 ++---
libsanitizer/sanitizer_common/sanitizer_platform.h | 2 +-
2 files changed, 11 insertions(+), 8 deletions(-)
diff --git a/libsanitizer/sanitizer_common/sanitizer_linux.cpp
b/libsanitizer/sanitizer_common/sanitizer_linux.cpp
index e2c32d679ad..5ba033492e7 100644
--- a/libsanitizer/sanitizer_common/sanitizer_linux.cpp
+++ b/libsanitizer/sanitizer_common/sanitizer_linux.cpp
@@ -34,7 +34,7 @@
// format. Struct kernel_stat is defined as 'struct stat' in asm/stat.h. To
// access stat from asm/stat.h, without conflicting with definition in
// sys/stat.h, we use this trick.
-#if defined(__mips64)
+#if SANITIZER_MIPS64
#include
#include
#define stat kernel_stat
@@ -124,8 +124,9 @@ const int FUTEX_WAKE_PRIVATE = FUTEX_WAKE |
FUTEX_PRIVATE_FLAG;
// Are we using 32-bit or 64-bit Linux syscalls?
// x32 (which defines __x86_64__) has SANITIZER_WORDSIZE == 32
// but it still needs to use 64-bit syscalls.
-#if SANITIZER_LINUX && (defined(__x86_64__) || defined(__powerpc64__) ||
\
-SANITIZER_WORDSIZE == 64)
+#if SANITIZER_LINUX && (defined(__x86_64__) || defined(__powerpc64__) || \
+SANITIZER_WORDSIZE == 64 || \
+(defined(__mips__) && _MIPS_SIM == _ABIN32))
# define SANITIZER_LINUX_USES_64BIT_SYSCALLS 1
#else
# define SANITIZER_LINUX_USES_64BIT_SYSCALLS 0
@@ -289,7 +290,7 @@ static void stat64_to_stat(struct stat64 *in, struct stat
*out) {
}
#endif
-#if defined(__mips64)
+#if SANITIZER_MIPS64
// Undefine compatibility macros from
// so that they would not clash with the kernel_stat
// st_[a|m|c]time fields
@@ -343,7 +344,8 @@ uptr internal_stat(const char *path, void *buf) {
#if SANITIZER_FREEBSD
return internal_syscall(SYSCALL(fstatat), AT_FDCWD, (uptr)path, (uptr)buf,
0);
#elif SANITIZER_LINUX
-# if SANITIZER_WORDSIZE == 64 || SANITIZER_X32
+# if SANITIZER_WORDSIZE == 64 || SANITIZER_X32 || \
+ (defined(__mips__) && _MIPS_SIM == _ABIN32)
return internal_syscall(SYSCALL(newfstatat), AT_FDCWD, (uptr)path, (uptr)buf,
0);
# else
@@ -366,7 +368,8 @@ uptr internal_lstat(const char *path, void *buf) {
return internal_syscall(SYSCALL(fstatat), AT_FDCWD, (uptr)path, (uptr)buf,
AT_SYMLINK_NOFOLLOW);
#elif SANITIZER_LINUX
-# if defined(_LP64) || SANITIZER_X32
+# if defined(_LP64) || SANITIZER_X32 || \
+ (defined(__mips__) && _MIPS_SIM == _ABIN32)
return internal_syscall(SYSCALL(newfstatat), AT_FDCWD, (uptr)path, (uptr)buf,
AT_SYMLINK_NOFOLLOW);
# else
@@ -1053,7 +1056,7 @@ uptr GetMaxVirtualAddress() {
return (1ULL << (MostSignificantSetBitIndex(GET_CURRENT_FRAME()) + 1)) - 1;
#elif SANITIZER_RISCV64
return (1ULL << 38) - 1;
-# elif defined(__mips64)
+# elif SANITIZER_MIPS64
return (1ULL << 40) - 1; // 0x00ffUL;
# elif defined(__s390x__)
return (1ULL << 53) - 1; // 0x001fUL;
diff --git a/libsanitizer/sanitizer_common/sanitizer_platform.h
b/libsanitizer/sanitizer_common/sanitizer_platform.h
index 8fe0d831431..8bd9a327623 100644
--- a/libsanitizer/sanitizer_common/sanitizer_platform.h
+++ b/libsanitizer/sanitizer_common/sanitizer_platform.h
@@ -159,7 +159,7 @@
#if defined(__mips__)
# define SANITIZER_MIPS 1
-# if defined(__mips64)
+# if defined(__mips64) && _MIPS_SIM == _ABI64
#define SANITIZER_MIPS32 0
#define SANITIZER_MIPS64 1
# else
---
Mips: Fix kernel_stat structure size
Fix kernel_stat structure size for non-Android 32-bit Mips. LLVM currently has this value for the kernel_stat structure size, as per compiler-rt/lib/sanitizer-common/sanitizer_platform_limits_posix.h. This also resolves one of the build issues for non-Android 32-bit Mips. libsanitizer/ChangeLog: * sanitizer_common/sanitizer_platform_limits_posix.h: Fix kernel_stat structure size for non-Android 32-bit Mips. --- libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h b/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h index 89772a7e5c0..62a99035db3 100644 --- a/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h +++ b/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h @@ -83,7 +83,7 @@ const unsigned struct_kernel_stat64_sz = 104; #elif defined(__mips__) const unsigned struct_kernel_stat_sz = SANITIZER_ANDROID ? FIRST_32_SECOND_64(104, 128) - : FIRST_32_SECOND_64(144, 216); + : FIRST_32_SECOND_64(160, 216); const unsigned struct_kernel_stat64_sz = 104; #elif defined(__s390__) && !defined(__s390x__) const unsigned struct_kernel_stat_sz = 64; ---
[PATCH] libsanitizer: Fix linkage errors for cross toolchains
When we use cross toolchains, in which the GCC libraries are not installed
within a designated system root, the shared sanitizer libraries link against
libstdc++.so* within the same libraries. This directory, however, is not in
RPATH, so attempting to build a dynamically linked application with
-fsanitize=... gives a linkage error.
More information can be found here:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69839.
gcc/ChangeLog:
* gcc.c (LIBSAN_RPATH): New macro.
(LIBASAN_SPEC): Add LIBSAN_RPATH.
(LIBUBSAN_SPEC): Likewise.
(LIBTSAN_SPEC): Likewise.
(LIBLSAN_SPEC): Likewise.
libsanitizer/ChangeLog:
* configure.ac (link_libsan_rpath): New config variable.
* libsanitizer.spec.in (link_libsan_rpath): New spec.
* configure (link_libsan_rpath): New config variable.
* Makefile.in (link_libsan_rpath): Define new Makefile variable.
* asan/Makefile.in: Likewise.
* interception/Makefile.in: Likewise.
* libbacktrace/Makefile.in: Likewise.
* lsan/Makefile.in: Likewise.
* sanitizer_common/Makefile.in: Likewise.
* tsan/Makefile.in: Likewise.
* ubsan/Makefile.in: Likewise.
* hwasan/Makefile.in: Likewise.
---
gcc/gcc.cc| 20
libsanitizer/Makefile.in | 1 +
libsanitizer/asan/Makefile.in | 1 +
libsanitizer/configure| 10 --
libsanitizer/configure.ac | 7 +++
libsanitizer/hwasan/Makefile.in | 1 +
libsanitizer/interception/Makefile.in | 1 +
libsanitizer/libbacktrace/Makefile.in | 1 +
libsanitizer/libsanitizer.spec.in | 2 ++
libsanitizer/lsan/Makefile.in | 1 +
libsanitizer/sanitizer_common/Makefile.in | 1 +
libsanitizer/tsan/Makefile.in | 1 +
libsanitizer/ubsan/Makefile.in| 1 +
13 files changed, 38 insertions(+), 10 deletions(-)
diff --git a/gcc/gcc.cc b/gcc/gcc.cc
index 299e09c4f54..37ff75f1ad5 100644
--- a/gcc/gcc.cc
+++ b/gcc/gcc.cc
@@ -738,17 +738,21 @@ proper position among the other output files. */
#define STACK_SPLIT_SPEC " %{fsplit-stack: --wrap=pthread_create}"
#endif
+#ifndef LIBSAN_RPATH
+#define LIBSAN_RPATH " %:include(libsanitizer.spec)%(link_libsan_rpath)"
+#endif
+
#ifndef LIBASAN_SPEC
#define STATIC_LIBASAN_LIBS \
" %{static-libasan|static:%:include(libsanitizer.spec)%(link_libasan)}"
#ifdef LIBASAN_EARLY_SPEC
-#define LIBASAN_SPEC STATIC_LIBASAN_LIBS
+#define LIBASAN_SPEC STATIC_LIBASAN_LIBS LIBSAN_RPATH
#elif defined(HAVE_LD_STATIC_DYNAMIC)
#define LIBASAN_SPEC "%{static-libasan:" LD_STATIC_OPTION \
"} -lasan %{static-libasan:" LD_DYNAMIC_OPTION "}" \
STATIC_LIBASAN_LIBS
#else
-#define LIBASAN_SPEC "-lasan" STATIC_LIBASAN_LIBS
+#define LIBASAN_SPEC "-lasan" STATIC_LIBASAN_LIBS LIBSAN_RPATH
#endif
#endif
@@ -778,13 +782,13 @@ proper position among the other output files. */
#define STATIC_LIBTSAN_LIBS \
" %{static-libtsan|static:%:include(libsanitizer.spec)%(link_libtsan)}"
#ifdef LIBTSAN_EARLY_SPEC
-#define LIBTSAN_SPEC STATIC_LIBTSAN_LIBS
+#define LIBTSAN_SPEC STATIC_LIBTSAN_LIBS LIBSAN_RPATH
#elif defined(HAVE_LD_STATIC_DYNAMIC)
#define LIBTSAN_SPEC "%{static-libtsan:" LD_STATIC_OPTION \
"} -ltsan %{static-libtsan:" LD_DYNAMIC_OPTION "}" \
STATIC_LIBTSAN_LIBS
#else
-#define LIBTSAN_SPEC "-ltsan" STATIC_LIBTSAN_LIBS
+#define LIBTSAN_SPEC "-ltsan" STATIC_LIBTSAN_LIBS LIBSAN_RPATH
#endif
#endif
@@ -793,7 +797,7 @@ proper position among the other output files. */
#endif
#ifndef LIBLSAN_SPEC
-#define STATIC_LIBLSAN_LIBS \
+#define STATIC_LIBLSAN_LIBS LIBSAN_RPATH \
" %{static-liblsan|static:%:include(libsanitizer.spec)%(link_liblsan)}"
#ifdef LIBLSAN_EARLY_SPEC
#define LIBLSAN_SPEC STATIC_LIBLSAN_LIBS
@@ -802,7 +806,7 @@ proper position among the other output files. */
"} -llsan %{static-liblsan:" LD_DYNAMIC_OPTION "}" \
STATIC_LIBLSAN_LIBS
#else
-#define LIBLSAN_SPEC "-llsan" STATIC_LIBLSAN_LIBS
+#define LIBLSAN_SPEC "-llsan" STATIC_LIBLSAN_LIBS LIBSAN_RPATH
#endif
#endif
@@ -816,9 +820,9 @@ proper position among the other output files. */
#ifdef HAVE_LD_STATIC_DYNAMIC
#define LIBUBSAN_SPEC "%{static-libubsan:" LD_STATIC_OPTION \
"} -lubsan %{static-libubsan:" LD_DYNAMIC_OPTION "}" \
-STATIC_LIBUBSAN_LIBS
+STATIC_LIBUBSAN_LIBS LIBSAN_RPATH
#else
-#define LIBUBSAN_SPEC "-lubsan" STATIC_LIBUBSAN_LIBS
+#define LIBUBSAN_SPEC "-lubsan" STATIC_LIBUBSAN_LIBS LIBSAN_RPATH
#endif
#endif
diff --git a/libsanitizer/Makefile.in b/libsanitizer/Makefile.in
index 65e7f2e9553..ef71407a512 100644
--- a/libsanitizer/Makefile.in
+++ b/libsanitizer/Makefile.in
@@ -333,6 +333,7 @@ libexecdir = @libexecdir@
link_libasan =
[PATCH] Mips: Resolve build issues for the n32 ABI
Building the ASAN for the n32 MIPS ABI currently fails, due to a few reasons:
- defined(__mips64), which is set solely based on the architecture type
(32-bit/64-bit), was still used in some places.
Therefore, defined(__mips64) is swapped with SANITIZER_MIPS64, which takes the
ABI into account as well - defined(__mips64) && _MIPS_SIM == ABI64.
- The n32 ABI still uses 64-bit *Linux* system calls, even though the word size
is 32 bits.
- After the transition to canonical system calls
(https://reviews.llvm.org/D124212), the n32 ABI still didn't use them, even
though they are supported,
as per
https://github.com/torvalds/linux/blob/master/arch/mips/kernel/syscalls/syscall_n32.tbl.
- struct_kernel_stat_sz was not updated after being changed in LLVM's source
tree.
See https://reviews.llvm.org/D127098.
libsanitizer/ChangeLog:
* sanitizer_common/sanitizer_linux.cpp (defined): Resolve
ASAN build issues for the n32 ABI.
* sanitizer_common/sanitizer_platform.h (defined): Likewise.
* sanitizer_common/sanitizer_platform_limits_posix.h: Likewise.
---
libsanitizer/sanitizer_common/sanitizer_linux.cpp | 17
++---
libsanitizer/sanitizer_common/sanitizer_platform.h | 2 +-
libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h | 2 +-
3 files changed, 12 insertions(+), 9 deletions(-)
diff --git a/libsanitizer/sanitizer_common/sanitizer_linux.cpp
b/libsanitizer/sanitizer_common/sanitizer_linux.cpp
index e2c32d679ad..5ba033492e7 100644
--- a/libsanitizer/sanitizer_common/sanitizer_linux.cpp
+++ b/libsanitizer/sanitizer_common/sanitizer_linux.cpp
@@ -34,7 +34,7 @@
// format. Struct kernel_stat is defined as 'struct stat' in asm/stat.h. To
// access stat from asm/stat.h, without conflicting with definition in
// sys/stat.h, we use this trick.
-#if defined(__mips64)
+#if SANITIZER_MIPS64
#include
#include
#define stat kernel_stat
@@ -124,8 +124,9 @@ const int FUTEX_WAKE_PRIVATE = FUTEX_WAKE |
FUTEX_PRIVATE_FLAG;
// Are we using 32-bit or 64-bit Linux syscalls?
// x32 (which defines __x86_64__) has SANITIZER_WORDSIZE == 32
// but it still needs to use 64-bit syscalls.
-#if SANITIZER_LINUX && (defined(__x86_64__) || defined(__powerpc64__) ||
\
-SANITIZER_WORDSIZE == 64)
+#if SANITIZER_LINUX && (defined(__x86_64__) || defined(__powerpc64__) || \
+SANITIZER_WORDSIZE == 64 || \
+(defined(__mips__) && _MIPS_SIM == _ABIN32))
# define SANITIZER_LINUX_USES_64BIT_SYSCALLS 1
#else
# define SANITIZER_LINUX_USES_64BIT_SYSCALLS 0
@@ -289,7 +290,7 @@ static void stat64_to_stat(struct stat64 *in, struct stat
*out) {
}
#endif
-#if defined(__mips64)
+#if SANITIZER_MIPS64
// Undefine compatibility macros from
// so that they would not clash with the kernel_stat
// st_[a|m|c]time fields
@@ -343,7 +344,8 @@ uptr internal_stat(const char *path, void *buf) {
#if SANITIZER_FREEBSD
return internal_syscall(SYSCALL(fstatat), AT_FDCWD, (uptr)path, (uptr)buf,
0);
#elif SANITIZER_LINUX
-# if SANITIZER_WORDSIZE == 64 || SANITIZER_X32
+# if SANITIZER_WORDSIZE == 64 || SANITIZER_X32 || \
+ (defined(__mips__) && _MIPS_SIM == _ABIN32)
return internal_syscall(SYSCALL(newfstatat), AT_FDCWD, (uptr)path, (uptr)buf,
0);
# else
@@ -366,7 +368,8 @@ uptr internal_lstat(const char *path, void *buf) {
return internal_syscall(SYSCALL(fstatat), AT_FDCWD, (uptr)path, (uptr)buf,
AT_SYMLINK_NOFOLLOW);
#elif SANITIZER_LINUX
-# if defined(_LP64) || SANITIZER_X32
+# if defined(_LP64) || SANITIZER_X32 || \
+ (defined(__mips__) && _MIPS_SIM == _ABIN32)
return internal_syscall(SYSCALL(newfstatat), AT_FDCWD, (uptr)path, (uptr)buf,
AT_SYMLINK_NOFOLLOW);
# else
@@ -1053,7 +1056,7 @@ uptr GetMaxVirtualAddress() {
return (1ULL << (MostSignificantSetBitIndex(GET_CURRENT_FRAME()) + 1)) - 1;
#elif SANITIZER_RISCV64
return (1ULL << 38) - 1;
-# elif defined(__mips64)
+# elif SANITIZER_MIPS64
return (1ULL << 40) - 1; // 0x00ffUL;
# elif defined(__s390x__)
return (1ULL << 53) - 1; // 0x001fUL;
diff --git a/libsanitizer/sanitizer_common/sanitizer_platform.h
b/libsanitizer/sanitizer_common/sanitizer_platform.h
index 8fe0d831431..8bd9a327623 100644
--- a/libsanitizer/sanitizer_common/sanitizer_platform.h
+++ b/libsanitizer/sanitizer_common/sanitizer_platform.h
@@ -159,7 +159,7 @@
#if defined(__mips__)
# define SANITIZER_MIPS 1
-# if defined(__mips64)
+# if defined(__mips64) && _MIPS_SIM == _ABI64
#define SANITIZER_MIPS32 0
#define SANITIZER_MIPS64 1
# else
diff --git a/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h
b/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.h
index 89772a7e5c0..62a99035db3 100644
---
Re: [PATCH] Mips: Enable asynchronous unwind tables with both ASAN and TSAN
Definitely, a patch is on the way. From: Xi Ruoyao Sent: Tuesday, June 7, 2022 10:20 AM To: Dimitrije Milosevic ; gcc-patches@gcc.gnu.org Cc: Djordje Todorovic Subject: Re: [PATCH] Mips: Enable asynchronous unwind tables with both ASAN and TSAN On Mon, 2022-05-30 at 07:10 +, Dimitrije Milosevic wrote: > Hi Xi, thanks for pointing this out. I'd definitely say that the > https://clang.llvm.org/docs/ThreadSanitizer.html documentation is > outdated. According > tohttps://github.com/google/sanitizers/wiki/ThreadSanitizerCppManual#s > upported-platforms TSAN is supported on Mips64. Furthermore, there are > actual code segments (in compiler-rt/lib/tsan/rtl/tsan_platforms.h, > for example) related to Mips64. > I didn't add the 64-bit target check, however. Here is the updated > version of the patch. Well, so should we add TSAN_SUPPORTED=yes for MIPS64 in libsanitizer/configure.tgt first? I'll try this on my MIPS64 in a few days. -- Xi Ruoyao School of Aerospace Science and Technology, Xidian University
Re: [PATCH] Mips: Fix the ASAN shadow offset hook for the n32 ABI
Correct, it should be committed very soon.
From: Xi Ruoyao
Sent: Tuesday, June 7, 2022 10:17 AM
To: Dimitrije Milosevic ;
gcc-patches@gcc.gnu.org
Cc: Djordje Todorovic
Subject: Re: [PATCH] Mips: Fix the ASAN shadow offset hook for the n32 ABI
On Mon, 2022-06-06 at 09:28 +, Dimitrije Milosevic wrote:
> Fix the ASAN shadow offset hook for the n32 ABI.
>
> gcc/ChangeLog:
>
> * config/mips/mips.cc (mips_asan_shadow_offset): Reformat
> to handle the N32 ABI.
> * config/mips/mips.h (SUBTARGET_SHADOW_OFFSET): Remove
> the macro, as it is not needed anymore.
>
> ---
>
> gcc/config/mips/mips.cc | 7 ++-
> gcc/config/mips/mips.h | 7 ---
> 2 files changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/gcc/config/mips/mips.cc b/gcc/config/mips/mips.cc
> index 2dce4007678..91e651c458e 100644
> --- a/gcc/config/mips/mips.cc
> +++ b/gcc/config/mips/mips.cc
> @@ -22745,7 +22745,12 @@ mips_constant_alignment (const_tree exp,
> HOST_WIDE_INT align)
> static unsigned HOST_WIDE_INT
> mips_asan_shadow_offset (void)
> {
> - return SUBTARGET_SHADOW_OFFSET;
> + if (mips_abi == ABI_N32)
> +return (HOST_WIDE_INT_1 << 29);
> + if (POINTER_SIZE == 64)
> +return (HOST_WIDE_INT_1 << 37);
> + else
> +return HOST_WIDE_INT_C (0x0aaa);
> }
>
> /* Implement TARGET_STARTING_FRAME_OFFSET. See
> mips_compute_frame_info
> diff --git a/gcc/config/mips/mips.h b/gcc/config/mips/mips.h
> index 858bbba3a36..0029864fdcd 100644
> --- a/gcc/config/mips/mips.h
> +++ b/gcc/config/mips/mips.h
> @@ -3463,10 +3463,3 @@ struct GTY(()) machine_function {
> && !TARGET_MICROMIPS && !TARGET_FIX_24K)
>
> #define NEED_INDICATE_EXEC_STACK 0
> -
> -/* Define the shadow offset for asan. Other OS's can override in the
> - respective tm.h files. */
> -#ifndef SUBTARGET_SHADOW_OFFSET
> -#define SUBTARGET_SHADOW_OFFSET \
> - (POINTER_SIZE == 64 ? HOST_WIDE_INT_1 << 37 : HOST_WIDE_INT_C
> (0x0aaa))
> -#endif
>
> ---
I think this depends on https://reviews.llvm.org/D127096 (not committed
yet)?
--
Xi Ruoyao
School of Aerospace Science and Technology, Xidian University
[PATCH] Mips: Fix the ASAN shadow offset hook for the n32 ABI
Fix the ASAN shadow offset hook for the n32 ABI.
gcc/ChangeLog:
* config/mips/mips.cc (mips_asan_shadow_offset): Reformat
to handle the N32 ABI.
* config/mips/mips.h (SUBTARGET_SHADOW_OFFSET): Remove
the macro, as it is not needed anymore.
---
gcc/config/mips/mips.cc | 7 ++-
gcc/config/mips/mips.h | 7 ---
2 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/gcc/config/mips/mips.cc b/gcc/config/mips/mips.cc
index 2dce4007678..91e651c458e 100644
--- a/gcc/config/mips/mips.cc
+++ b/gcc/config/mips/mips.cc
@@ -22745,7 +22745,12 @@ mips_constant_alignment (const_tree exp, HOST_WIDE_INT
align)
static unsigned HOST_WIDE_INT
mips_asan_shadow_offset (void)
{
- return SUBTARGET_SHADOW_OFFSET;
+ if (mips_abi == ABI_N32)
+return (HOST_WIDE_INT_1 << 29);
+ if (POINTER_SIZE == 64)
+return (HOST_WIDE_INT_1 << 37);
+ else
+return HOST_WIDE_INT_C (0x0aaa);
}
/* Implement TARGET_STARTING_FRAME_OFFSET. See mips_compute_frame_info
diff --git a/gcc/config/mips/mips.h b/gcc/config/mips/mips.h
index 858bbba3a36..0029864fdcd 100644
--- a/gcc/config/mips/mips.h
+++ b/gcc/config/mips/mips.h
@@ -3463,10 +3463,3 @@ struct GTY(()) machine_function {
&& !TARGET_MICROMIPS && !TARGET_FIX_24K)
#define NEED_INDICATE_EXEC_STACK 0
-
-/* Define the shadow offset for asan. Other OS's can override in the
- respective tm.h files. */
-#ifndef SUBTARGET_SHADOW_OFFSET
-#define SUBTARGET_SHADOW_OFFSET \
- (POINTER_SIZE == 64 ? HOST_WIDE_INT_1 << 37 : HOST_WIDE_INT_C (0x0aaa))
-#endif
---
Re: [PATCH] Mips: Enable asynchronous unwind tables with both ASAN and TSAN
Hi Xi, thanks for pointing this out. I'd definitely say that the https://clang.llvm.org/docs/ThreadSanitizer.html documentation is outdated. According to https://github.com/google/sanitizers/wiki/ThreadSanitizerCppManual#supported-platforms TSAN is supported on Mips64. Furthermore, there are actual code segments (in compiler-rt/lib/tsan/rtl/tsan_platforms.h, for example) related to Mips64. I didn't add the 64-bit target check, however. Here is the updated version of the patch. gcc/ChangeLog: * config/mips/mips.cc (mips_option_override): Enable asyncronous unwind tables with both ASAN and TSAN. --- gcc/config/mips/mips.cc | 14 ++ 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/gcc/config/mips/mips.cc b/gcc/config/mips/mips.cc index e64928f4113..2dce4007678 100644 --- a/gcc/config/mips/mips.cc +++ b/gcc/config/mips/mips.cc @@ -20115,10 +20115,16 @@ mips_option_override (void) target_flags |= MASK_64BIT; } - /* -fsanitize=address needs to turn on -fasynchronous-unwind-tables in - order for tracebacks to be complete but not if any - -fasynchronous-unwind-table were already specified. */ - if (flag_sanitize & SANITIZE_USER_ADDRESS + /* -fsanitize=address, and -fsanitize=thread need to turn + on -fasynchronous-unwind-tables in order for tracebacks + to be complete but not if any -fasynchronous-unwind-table + were already specified. */ + /* FIXME: HWSAN is currently only available on AArch64, + but could also utilize -fasynchronous-unwind-tables. + FIXME: We would also like to check if -ffreestanding is passed in. + However, it is only available in C-ish frontends. */ + if (((flag_sanitize & SANITIZE_USER_ADDRESS) + || (TARGET_64BIT && (flag_sanitize & SANITIZE_THREAD))) && !global_options_set.x_flag_asynchronous_unwind_tables) flag_asynchronous_unwind_tables = 1; --- From: Xi Ruoyao Sent: Saturday, May 28, 2022 12:30 PM To: Dimitrije Milosevic ; gcc-patches@gcc.gnu.org Cc: Djordje Todorovic Subject: Re: [PATCH] Mips: Enable asynchronous unwind tables with both ASAN and TSAN On Thu, 2022-05-26 at 14:18 +, Dimitrije Milosevic wrote: > Enable asynchronous unwind tables with both ASAN and TSAN for correct > back-trace. > LLVM currently enables asynchronous unwind tables for: ASAN, HWSAN, TSAN, > MSAN, and DFSAN. > HWSAN is currently available only on AArch64, while MSAN and DFSAN are not > available at all. > Also, LLVM checks is '-ffreestanding' is not passed in. '-ffreestanding' is > only available in C-family frontends, hence why no such check is included. > TODO: Not sure if any tests should be added. According to https://clang.llvm.org/docs/ThreadSanitizer.html, TSAN is not supported on MIPS. Is this doc outdated? -- Xi Ruoyao School of Aerospace Science and Technology, Xidian University
[PATCH] Mips: Enable asynchronous unwind tables with both ASAN and LSAN
Enable asynchronous unwind tables with both ASAN and TSAN for correct back-trace. LLVM currently enables asynchronous unwind tables for: ASAN, HWSAN, TSAN, MSAN, and DFSAN. HWSAN is currently available only on AArch64, while MSAN and DFSAN are not available at all. Also, LLVM checks is '-ffreestanding' is not passed in. '-ffreestanding' is only available in C-family frontends, hence why no such check is included. TODO: Not sure if any tests should be added. gcc/ChangeLog: * config/mips/mips.cc (mips_option_override): Enable asyncronous unwind tables with both ASAN and TSAN. --- gcc/config/mips/mips.cc | 13 + 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/gcc/config/mips/mips.cc b/gcc/config/mips/mips.cc index e64928f4113..ea2a038216c 100644 --- a/gcc/config/mips/mips.cc +++ b/gcc/config/mips/mips.cc @@ -20115,10 +20115,15 @@ mips_option_override (void) target_flags |= MASK_64BIT; } - /* -fsanitize=address needs to turn on -fasynchronous-unwind-tables in - order for tracebacks to be complete but not if any - -fasynchronous-unwind-table were already specified. */ - if (flag_sanitize & SANITIZE_USER_ADDRESS + /* -fsanitize=address, and -fsanitize=thread need to turn + on -fasynchronous-unwind-tables in order for tracebacks + to be complete but not if any -fasynchronous-unwind-table + were already specified. */ + /* FIXME: HWSAN is currently only available on AArch64, + but could also utilize -fasynchronous-unwind-tables. + FIXME: We would also like to check if -ffreestanding is passed in. + However, it is only available in C-ish frontends. */ + if (flag_sanitize & (SANITIZE_USER_ADDRESS | SANITIZE_THREAD) && !global_options_set.x_flag_asynchronous_unwind_tables) flag_asynchronous_unwind_tables = 1; ---
