Re: [XEN PATCH v9 08/30] build: fix enforce unique symbols for recent clang version

From: Jan Beulich <jbeulich@suse.com>
To: Anthony PERARD <anthony.perard@citrix.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"George Dunlap" <george.dunlap@citrix.com>,
	"Julien Grall" <julien@xen.org>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"Wei Liu" <wl@xen.org>,
	xen-devel@lists.xenproject.org
Subject: Re: [XEN PATCH v9 08/30] build: fix enforce unique symbols for recent clang version
Date: Mon, 31 Jan 2022 09:59:16 +0100	[thread overview]
Message-ID: <2340292f-54d3-62f8-1d5b-b54e1c3728e9@suse.com> (raw)
In-Reply-To: <YfQRQSwuJW+XOWYa@perard>

On 28.01.2022 16:52, Anthony PERARD wrote:
> On Fri, Jan 28, 2022 at 01:43:38PM +0100, Jan Beulich wrote:
>> On 28.01.2022 13:03, Anthony PERARD wrote:
>>> On Thu, Jan 27, 2022 at 04:57:20PM +0100, Jan Beulich wrote:
>>>> On 25.01.2022 12:00, Anthony PERARD wrote:
>>>>> clang 6.0 and newer behave like gcc in regards for the FILE symbol, so
>>>>> only the filename rather than the full path to the source file.
>>>>>
>>>>> clang 3.8.1-24 (in our debian:stretch container) and 3.5.0-10
>>>>> (in our debian:jessie container) do store the full path to the source
>>>>> file in the FILE symbol.
>>>>>
>>>>> Also, based on commit 81ecb38b83 ("build: provide option to
>>>>> disambiguate symbol names"), which were using clang 5, the change of
>>>>> behavior likely happened in clang 6.0.
>>>>>
>>>>> This means that we also need to check clang version to figure out
>>>>> which command we need to use to redefine symbol.
>>>>>
>>>>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>>>>
>>>> The "likely" in the description still worries me some. Roger, would
>>>> you happen to know, or know of a way to find out for sure ("sure"
>>>> not meaning to exclude the usual risk associated with version
>>>> number checks)?
>>>
>>> I found f5040b9685a7 ("Make .file directive to have basename only") as
>>> part of LLVM's "release/6.x" branch (and "llvmorg-6.0.0" tag), but not
>>> in "release/5.x".
>>>
>>> https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fllvm%2Fllvm-project%2Fcommit%2Ff5040b9685a760e584c576e9185295e54635d51e&amp;data=04%7C01%7Canthony.perard%40citrix.com%7C1ce7898a15bb4024260008d9e25be6f9%7C335836de42ef43a2b145348c2ee9ca5b%7C0%7C0%7C637789706644173026%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=V73NmkJWAHpqlzY9sAysf6%2Fw7q8ik6twT6lMLgglR3s%3D&amp;reserved=0
>>>
>>> This patch would seems to be the one changing the behavior. This still
>>> suggest clang 6.0.
>>
>> Oh, thanks for digging this out. May I suggest to replace (or delete)
>> "likely" then in the description?
> 
> Maybe something like that? Or just delete the word might be enough.
> 
>     Also we have commit 81ecb38b83 ("build: provide option to
>     disambiguate symbol names") which were using clang 5, and LLVM's
>     commit f5040b9685a7 [1] ("Make .file directive to have basename
>     only") which is part of "llvmorg-6.0.0" tag but not "release/5.x"
>     branch. Both suggest that clang change of behavior happened with
>     clang 6.0.
> 
>     [1] https://github.com/llvm/llvm-project/commit/f5040b9685a760e584c576e9185295e54635d51e

This sounds better to me than just dropping the one word.

Jan