-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Swift: Make tracer config handle resource-dirs passed to clang #20639
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates the Swift tracer configuration to properly handle resource directories passed to clang through the -Xcc flag. The changes allow the tracer to extract the correct Swift resource directory even when clang-specific resource directories are specified.
- Enhanced resource directory detection to handle
-Xccprefixed arguments - Added logic to strip
/clangsuffix from clang resource directories to derive Swift resource directories - Fixed a minor string concatenation issue in the fallback resource directory path
swift/tools/tracing-config.lua
Outdated
| if args[resource_dir_index + 1] and args[resource_dir_index + 1] ~= '-Xcc' then | ||
| return args[resource_dir_index + 1] | ||
| elseif args[resource_dir_index + 2] then | ||
| local clang_index = string.find(args[resource_dir_index + 2], "/clang$") | ||
| if clang_index and clang_index - 1 > 0 then | ||
| return string.sub(args[resource_dir_index + 2], 1, clang_index - 1) | ||
| end | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: while this works, it's slightly at odds with the comment, that speaks about an -Xcc preceding -resource-dir. So maybe
| if args[resource_dir_index + 1] and args[resource_dir_index + 1] ~= '-Xcc' then | |
| return args[resource_dir_index + 1] | |
| elseif args[resource_dir_index + 2] then | |
| local clang_index = string.find(args[resource_dir_index + 2], "/clang$") | |
| if clang_index and clang_index - 1 > 0 then | |
| return string.sub(args[resource_dir_index + 2], 1, clang_index - 1) | |
| end | |
| end | |
| if ~(args[resource_dir_index - 1] and args[resource_dir_index - 1] == '-Xcc') then | |
| return args[resource_dir_index + 1] | |
| elseif args[resource_dir_index + 1] and args[resource_dir_index + 1] == '-Xcc' and args[resource_dir_index + 2] then | |
| local clang_index = string.find(args[resource_dir_index + 2], "/clang$") | |
| if clang_index and clang_index - 1 > 0 then | |
| return string.sub(args[resource_dir_index + 2], 1, clang_index - 1) | |
| end | |
| end |
would be more semantically correct in a pedantic way?
(looking back, it might also be good to somehow shorten the resource_dir_index name, I wouldn't mind just pos or found)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(notice one doesn't need to do bound checking in Lua, because referencing out-of-bounds indexes in a table will just give nil, including on 0 and negative indexes)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be more semantically correct in a pedantic way?
In the if-case do we still need to check that args[resource_dir_index + 1] exists? Just to be sure we end up in the backup case if args[resource_dir_index + 1] is missing
(notice one doesn't need to do bound checking in Lua, because referencing out-of-bounds indexes in a table will just give
nil, including on0and negative indexes)
Is there specific code you're referring to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the if-case do we still need to check that
args[resource_dir_index + 1]exists? Just to be sure we end up in the backup case ifargs[resource_dir_index + 1]is missing
I was thinking about that. Even if we did the fallback on a truncated args vector ending with -resource-dir (which is an ill-formed command line we shouldn't ever encounter), we currently return nil here which is probably a bad thing. So maybe the full pedantic code should be
local found = indexOf(args, '-resource-dir')
if found and args[found + 1] then
if args[found - 1] ~= '-Xcc' then
return args[found + 1]
elseif args[found + 1] == '-Xcc' and args[found + 2] then
local clang_index = string.find(args[found + 2], "/clang$")
if clang_index and clang_index - 1 > 0 then
return string.sub(args[found + 2], 1, clang_index - 1)
end
end
endthis goes for the fallback early if -resource-dir is the last argument, takes its direct successor if the preceding one is not -Xcc (which also covers the case in which it is nil, when found == 1), and takes the clang resource dir only for a well formed -Xcc -resource-dir -Xcc <path>/clang sequence. And it covers the devilish case in which someone very sadistic is pointing -resource-dir to a local -Xcc directory 😆
Is there specific code you're referring to?
I was referring to my suggestion, where doing args[pos - 1] == '-Xcc' doesn't need bound checking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've implemented your fully pedantic review suggestion 😄
redsun82
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks a lot!
No description provided.