[COMMITTED] rust_debug: Cast size_t values to unsigned long before printing.

[COMMITTED] rust_debug: Cast size_t values to unsigned long before printing.

[Arthur Cohen]

Using %lu to format size_t values breaks 32 bit targets, and %zu is not
supported by one of the hosts GCC aims to support - HPUX

gcc/rust/ChangeLog:

	* backend/rust-compile-base.cc (HIRCompileBase::resolve_method_address):
	Cast size_t value to unsigned long.
	* expand/rust-proc-macro.cc (load_macros): Likewise.
	* typecheck/rust-hir-type-check-expr.cc (TypeCheckExpr::visit): Likewise.
---
 gcc/rust/backend/rust-compile-base.cc          | 3 ++-
 gcc/rust/expand/rust-proc-macro.cc             | 2 +-
 gcc/rust/typecheck/rust-hir-type-check-expr.cc | 4 ++--
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/gcc/rust/backend/rust-compile-base.cc b/gcc/rust/backend/rust-compile-base.cc
index b4a3685ad93..ae9f6707b72 100644
--- a/gcc/rust/backend/rust-compile-base.cc
+++ b/gcc/rust/backend/rust-compile-base.cc
@@ -965,7 +965,8 @@ HIRCompileBase::resolve_method_address (TyTy::FnType *fntype,
     }
 
   const Resolver::PathProbeCandidate *selectedCandidate = nullptr;
-  rust_debug_loc (expr_locus, "resolved to %lu candidates", candidates.size ());
+  rust_debug_loc (expr_locus, "resolved to %lu candidates",
+		  (unsigned long) candidates.size ());
 
   // filter for the possible case of non fn type items
   std::set<Resolver::PathProbeCandidate> filteredFunctionCandidates;
diff --git a/gcc/rust/expand/rust-proc-macro.cc b/gcc/rust/expand/rust-proc-macro.cc
index e8618485b71..09680733e98 100644
--- a/gcc/rust/expand/rust-proc-macro.cc
+++ b/gcc/rust/expand/rust-proc-macro.cc
@@ -171,7 +171,7 @@ load_macros (std::string path)
   if (array == nullptr)
     return {};
 
-  rust_debug ("Found %lu procedural macros", array->length);
+  rust_debug ("Found %lu procedural macros", (unsigned long) array->length);
 
   return std::vector<ProcMacro::Procmacro> (array->macros,
 					    array->macros + array->length);
diff --git a/gcc/rust/typecheck/rust-hir-type-check-expr.cc b/gcc/rust/typecheck/rust-hir-type-check-expr.cc
index 9dbf657958d..030e5f1b63c 100644
--- a/gcc/rust/typecheck/rust-hir-type-check-expr.cc
+++ b/gcc/rust/typecheck/rust-hir-type-check-expr.cc
@@ -1122,10 +1122,10 @@ TypeCheckExpr::visit (HIR::MethodCallExpr &expr)
 
   auto candidate = *candidates.begin ();
   rust_debug_loc (expr.get_method_name ().get_locus (),
-		  "resolved method to: {%u} {%s} with [%zu] adjustments",
+		  "resolved method to: {%u} {%s} with [%lu] adjustments",
 		  candidate.candidate.ty->get_ref (),
 		  candidate.candidate.ty->debug_str ().c_str (),
-		  candidate.adjustments.size ());
+		  (unsigned long) candidate.adjustments.size ());
 
   // Get the adjusted self
   Adjuster adj (receiver_tyty);
-- 
2.42.1

Re: [COMMITTED] rust_debug: Cast size_t values to unsigned long before printing.

[Rainer Orth]

Arthur Cohen <arthur.cohen@embecosm.com> writes:

> Using %lu to format size_t values breaks 32 bit targets, and %zu is not
> supported by one of the hosts GCC aims to support - HPUX

But we do have uses of %zu in gcc/rust already!

> diff --git a/gcc/rust/expand/rust-proc-macro.cc b/gcc/rust/expand/rust-proc-macro.cc
> index e8618485b71..09680733e98 100644
> --- a/gcc/rust/expand/rust-proc-macro.cc
> +++ b/gcc/rust/expand/rust-proc-macro.cc
> @@ -171,7 +171,7 @@ load_macros (std::string path)
>    if (array == nullptr)
>      return {};
>  
> -  rust_debug ("Found %lu procedural macros", array->length);
> +  rust_debug ("Found %lu procedural macros", (unsigned long) array->length);

Not the best way either: array->length is std::uint64_t, so the format
should use

... %" PRIu64 " procedural...

instead.

I've attached my patch to PR rust/113461.

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [COMMITTED] rust_debug: Cast size_t values to unsigned long before printing.

[Rainer Orth]

Hi Arthur,

> Yes, I was talking about this on IRC the other day - if we do run in a
> situation where we have more than UINT32_MAX procedural macros in memory 
> we have big issues. These debug prints will probably end up getting removed
> soon as they clutter the output a lot for little information.

makes sense, especially if they break the build once in a while ;-)

> I don't mind doing it the right way for our regular prints, but we have not
> been using PRIu64 in our codebase so far, so I'd rather change all those
> incriminating format specifiers at once later down the line - this patch
> was pushed so that 32bit targets could bootstrap the Rust frontend for now.

Makes sense: using different styles throughout the codebase only creates
confusion.

On a related issue: didn't you have some 32-bit host in your CI?  I
remember having similar issues in the past which could easily be avoided
in advance this way.

Thanks.
        Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [COMMITTED] rust_debug: Cast size_t values to unsigned long before printing.

[Arthur Cohen]

Hi Rainer,

On 1/18/24 10:13, Rainer Orth wrote:
> Arthur Cohen <arthur.cohen@embecosm.com> writes:
> 
>> Using %lu to format size_t values breaks 32 bit targets, and %zu is not
>> supported by one of the hosts GCC aims to support - HPUX
> 
> But we do have uses of %zu in gcc/rust already!
> 
>> diff --git a/gcc/rust/expand/rust-proc-macro.cc b/gcc/rust/expand/rust-proc-macro.cc
>> index e8618485b71..09680733e98 100644
>> --- a/gcc/rust/expand/rust-proc-macro.cc
>> +++ b/gcc/rust/expand/rust-proc-macro.cc
>> @@ -171,7 +171,7 @@ load_macros (std::string path)
>>     if (array == nullptr)
>>       return {};
>>   
>> -  rust_debug ("Found %lu procedural macros", array->length);
>> +  rust_debug ("Found %lu procedural macros", (unsigned long) array->length);
> 
> Not the best way either: array->length is std::uint64_t, so the format
> should use
> 
> ... %" PRIu64 " procedural...
> 
> instead.
> 
> I've attached my patch to PR rust/113461.

Yes, I was talking about this on IRC the other day - if we do run in a 
situation where we have more than UINT32_MAX procedural macros in memory 
we have big issues. These debug prints will probably end up getting 
removed soon as they clutter the output a lot for little information.

I don't mind doing it the right way for our regular prints, but we have 
not been using PRIu64 in our codebase so far, so I'd rather change all 
those incriminating format specifiers at once later down the line - this 
patch was pushed so that 32bit targets could bootstrap the Rust frontend 
for now.

Best,

Arthur

> 	Rainer
> 

Re: [COMMITTED] rust_debug: Cast size_t values to unsigned long before printing.

[Iain Sandoe]

Hi Arthur,

> On 18 Jan 2024, at 10:30, Arthur Cohen <arthur.cohen@embecosm.com> wrote:

> On 1/18/24 10:13, Rainer Orth wrote:
>> Arthur Cohen <arthur.cohen@embecosm.com> writes:
>>> Using %lu to format size_t values breaks 32 bit targets, and %zu is not
>>> supported by one of the hosts GCC aims to support - HPUX
>> But we do have uses of %zu in gcc/rust already!
>>> diff --git a/gcc/rust/expand/rust-proc-macro.cc b/gcc/rust/expand/rust-proc-macro.cc
>>> index e8618485b71..09680733e98 100644
>>> --- a/gcc/rust/expand/rust-proc-macro.cc
>>> +++ b/gcc/rust/expand/rust-proc-macro.cc
>>> @@ -171,7 +171,7 @@ load_macros (std::string path)
>>>    if (array == nullptr)
>>>      return {};
>>>  -  rust_debug ("Found %lu procedural macros", array->length);
>>> +  rust_debug ("Found %lu procedural macros", (unsigned long) array->length);
>> Not the best way either: array->length is std::uint64_t, so the format
>> should use
>> ... %" PRIu64 " procedural...
>> instead.
>> I've attached my patch to PR rust/113461.
> 
> Yes, I was talking about this on IRC the other day - if we do run in a situation where we have more than UINT32_MAX procedural macros in memory we have big issues. These debug prints will probably end up getting removed soon as they clutter the output a lot for little information.
> 
> I don't mind doing it the right way for our regular prints, but we have not been using PRIu64 in our codebase so far, so I'd rather change all those incriminating format specifiers at once later down the line - this patch was pushed so that 32bit targets could bootstrap the Rust frontend for now.

For the sake of completeness, the issue does not just affect 32b hosts;  If a 64b host chooses (as Darwin does, so that 32b and 64b targets have the same representation) to make uint64_t “unsigned long long int”, then %lu breaks there too.
thanks
Iain

Re: [COMMITTED] rust_debug: Cast size_t values to unsigned long before printing.

[Arthur Cohen]

Hi Rainer,

On 1/18/24 10:34, Rainer Orth wrote:
> Hi Arthur,
> 
>> Yes, I was talking about this on IRC the other day - if we do run in a
>> situation where we have more than UINT32_MAX procedural macros in memory
>> we have big issues. These debug prints will probably end up getting removed
>> soon as they clutter the output a lot for little information.
> 
> makes sense, especially if they break the build once in a while ;-)
> 
>> I don't mind doing it the right way for our regular prints, but we have not
>> been using PRIu64 in our codebase so far, so I'd rather change all those
>> incriminating format specifiers at once later down the line - this patch
>> was pushed so that 32bit targets could bootstrap the Rust frontend for now.
> 
> Makes sense: using different styles throughout the codebase only creates
> confusion.
> 
> On a related issue: didn't you have some 32-bit host in your CI?  I
> remember having similar issues in the past which could easily be avoided
> in advance this way.

We do have 32 bits runners in the buildbot Mark takes care of, but they 
were not running bootstrap builds so this was getting ignored as it only 
produced a warning. Definitely something I want to fix quickly.

> 
> Thanks.
>          Rainer
> 

Re: [COMMITTED] rust_debug: Cast size_t values to unsigned long before printing.

[Arthur Cohen]

Hi Iain,

On 1/18/24 12:02, Iain Sandoe wrote:
> Hi Arthur,
> 
>> On 18 Jan 2024, at 10:30, Arthur Cohen <arthur.cohen@embecosm.com> wrote:
> 
>> On 1/18/24 10:13, Rainer Orth wrote:
>>> Arthur Cohen <arthur.cohen@embecosm.com> writes:
>>>> Using %lu to format size_t values breaks 32 bit targets, and %zu is not
>>>> supported by one of the hosts GCC aims to support - HPUX
>>> But we do have uses of %zu in gcc/rust already!
>>>> diff --git a/gcc/rust/expand/rust-proc-macro.cc b/gcc/rust/expand/rust-proc-macro.cc
>>>> index e8618485b71..09680733e98 100644
>>>> --- a/gcc/rust/expand/rust-proc-macro.cc
>>>> +++ b/gcc/rust/expand/rust-proc-macro.cc
>>>> @@ -171,7 +171,7 @@ load_macros (std::string path)
>>>>     if (array == nullptr)
>>>>       return {};
>>>>   -  rust_debug ("Found %lu procedural macros", array->length);
>>>> +  rust_debug ("Found %lu procedural macros", (unsigned long) array->length);
>>> Not the best way either: array->length is std::uint64_t, so the format
>>> should use
>>> ... %" PRIu64 " procedural...
>>> instead.
>>> I've attached my patch to PR rust/113461.
>>
>> Yes, I was talking about this on IRC the other day - if we do run in a situation where we have more than UINT32_MAX procedural macros in memory we have big issues. These debug prints will probably end up getting removed soon as they clutter the output a lot for little information.
>>
>> I don't mind doing it the right way for our regular prints, but we have not been using PRIu64 in our codebase so far, so I'd rather change all those incriminating format specifiers at once later down the line - this patch was pushed so that 32bit targets could bootstrap the Rust frontend for now.
> 
> For the sake of completeness, the issue does not just affect 32b hosts;  If a 64b host chooses (as Darwin does, so that 32b and 64b targets have the same representation) to make uint64_t “unsigned long long int”, then %lu breaks there too.
> thanks
> Iain
>

Thanks for the precision! I'll definitely be more careful moving forward.

Kindly,

Arthur