[Reproduce] Support linker scripts resolved through -L - #1763
[Reproduce] Support linker scripts resolved through -L#1763Rachit Mehta (rachitmeht) wants to merge 1 commit into
Conversation
373a85a to
3c0de8a
Compare
Capture linker scripts during early activation and rewrite -T paths to their mapped paths in reproduce response files. Add a regression test covering linker scripts found through a -L search directory. Resolves qualcomm#1454 Signed-off-by: Rachit Mehta <rachmeht@qti.qualcomm.com>
3c0de8a to
fe25f35
Compare
|
|
||
| // Capture scripts during early activation so the response file can rewrite | ||
| // -T paths that were resolved through a -L search directory. | ||
| addInputFileToTar(input, eld::MappingFile::LinkerScript); |
There was a problem hiding this comment.
It is being called for both early activation and late activation, right?
Why can't the response file rewrite -T paths that were resolved through -L subdirectory if we capture the information later in full activation?
There was a problem hiding this comment.
Yes, addInputFileToTar() may be called during both early and full activation. For a normal successful link, full activation usually happens and may capture the script later. The reason for keeping the early capture is to cover the broader --reproduce-on-fail case: any earlier input error, script error or interruption can prevent full activation from being reached.
There was a problem hiding this comment.
The reason for keeping the early capture is to cover the broader --reproduce-on-fail case: any earlier input error, script error or interruption can prevent full activation from being reached.
Then can you fix the comments above addInputFileToTar here? The comments suggests that early activation is somehow required when script is found in -L subdirectory.
| return false; | ||
| os << arg->getSpelling() << ' ' << *path << ' '; | ||
| bool rewritten = false; | ||
| for (size_t i = lastScriptId + 1; i < actions.size(); ++i) { |
There was a problem hiding this comment.
Why do we need to determine the script's InputFile positionally? Why does outputTar->rewritePath(...) does not work here?
There was a problem hiding this comment.
outputTar->rewritePath("script.t") does not find the mapping created for
"/work/A/lib/script.t" and falls back to "script.t"
So, The ScriptAction lookup is needed to obtain the mapped resolved path.
There was a problem hiding this comment.
Positionally determining InputFile will allow us to have args with same name but may resolve to different files. like
-L dir1 -T script.t -L dir2 -T script.t
There was a problem hiding this comment.
outputTar->rewritePath("script.t") does not find the mapping created for
"/work/A/lib/script.t" and falls back to "script.t"
Yes, that is the core issue that needs to be fixed.
What is the benefit of determining the InputFile positionally and making the code and the flow more complex as opposed to fixing outputTar->rewritePath(...) to function correctly here?
Positionally determining InputFile will allow us to have args with same name but may resolve to different files. like -L dir1 -T script.t -L dir2 -T script.t
Can you verify whether the 2 script.t here would resolve to different scripts if dir1 and dir2 both contains script.t?
There was a problem hiding this comment.
I verified, the 2 script.t will resolve to the first -L dir only. I was influenced by how we handle namespecs here.
I agree this approach doesn't fix outputTar->rewritePath(...).
But if we fix that we must handle LinkerScript seperately in that function to give correct path. The current fix effectively does that.
Will that be better design?
There was a problem hiding this comment.
But if we fix that we must handle LinkerScript seperately in that function to give correct path. The current fix effectively does that.
Why would we need to handle LinkerScript separately in that function? Can you think of a solution that fixes this problem in a more general way?
There was a problem hiding this comment.
So, The issue will be for every script ( version script & LinkerScript ) which excecutes ScriptAction::Activate()
Because it changes the file name by
Path = Res->native();
}
setFileName(Path);
So, the MappedPath contains the wrong Name for scripts came through -L.
But processReproduceOption() calls rewritePath() with the original argument spelling from the command line. Since rewritePath() only receives a string and does an exact lookup in
InputMap, it cannot know that script.t should correspond to /path/from/-L/script.t.
So we can either:
have conditional handling of these scripts before calling rewritePath(), or
we can have a different Reproduce Specific map which will have the original file name. (we only need to store original map in ScriptAction:: Activate())
| RUN: %link --no-threads -L %t.lib -T script.t %t.main.o -o %t.out --reproduce %t.tar --dump-response-file %t.response | ||
| RUN: %filecheck %s < %t.response | ||
| RUN: %mkdir %t.reproduce | ||
| RUN: %tar %gnutaropts -xf %t.tar -C %t.reproduce --strip-components=1 |
There was a problem hiding this comment.
tar .... -C is not compatible with Windows. you can cd into %t.reproduce first that way you don't need -C
There was a problem hiding this comment.
Sure, I will change it.
3 participants
Capture linker scripts during early activation and rewrite -T paths to their mapped paths in reproduce response files. Add a regression test covering linker scripts found through a -L search directory.
Resolves #1454