Skip to content

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.

Sign up for GitHub

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

Merged
jketema merged 2 commits into from
Oct 15, 2025
Merged

Swift: Make tracer config handle resource-dirs passed to clang #20639

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b6b3767
Swift: Make tracer config handle resource-dirs passed to clang
jketema Oct 14, 2025
9fc8faa
Swift: Address review comments
jketema Oct 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 20 additions & 10 deletions swift/tools/tracing-config.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,17 +57,27 @@ function RegisterExtractorPack(id)

-- xcodebuild does not always specify the -resource-dir in which case the compiler falls back
-- to a resource-dir based on its path. We want to know what is the original resource-dir in
-- all cases so that we can patch it with out own
-- all cases so that we can patch it with out own. When we see a -resource-dir preceded by
-- -Xcc this will be a resource-dir that is passed to clang. We can still obtain the swift
-- resource-dir in this case by skipping over the -Xcc that follows it and stripping off the
-- clang suffix from the path.
function find_original_resource_dir(compilerPath, args)
local resource_dir_index = indexOf(args, '-resource-dir')
if resource_dir_index and args[resource_dir_index + 1] then
return args[resource_dir_index + 1]
end
-- derive -resource-dir based on the compilerPath
-- e.g.: /usr/bin/swift-frontend -> /usr/bin/../lib/swift
local second_last_slash_index = string.find(compilerPath, "/[^/]*/[^/]*$")
local usr_dir = string.sub(compilerPath, 1, second_last_slash_index)
return usr_dir .. '/lib/swift'
local resource_dir_index = indexOf(args, '-resource-dir')
if resource_dir_index then
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
Copy link
Contributor

@redsun82 redsun82 Oct 14, 2025 •
edited
Loading

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

Suggested change
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)

Copy link
Contributor

@redsun82 redsun82 Oct 14, 2025

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)

Copy link
Contributor Author

@jketema jketema Oct 14, 2025

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 on 0 and negative indexes)

Is there specific code you're referring to?

Copy link
Contributor

@redsun82 redsun82 Oct 14, 2025

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 if args[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
        end

this 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

Copy link
Contributor Author

@jketema jketema Oct 15, 2025

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 😄

end
-- derive -resource-dir based on the compilerPath
-- e.g.: /usr/bin/swift-frontend -> /usr/bin/../lib/swift
local second_last_slash_index = string.find(compilerPath, "/[^/]*/[^/]*$")
local usr_dir = string.sub(compilerPath, 1, second_last_slash_index)
return usr_dir .. 'lib/swift'
end

-- replace or add our own resource directory
Expand Down
Loading