Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1774383002:
PNaCl Dynamic Linking: Force __pnacl_pso_root to be an external declaration. (Closed)
|
Created:
4 years, 9 months ago by Sean Klein Modified:
4 years, 9 months ago CC:
native-client-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/native_client/pnacl-subzero.git@fixes Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionPNaCl Dynamic Linking: Force __pnacl_pso_root to be an external declaration.
This CL updates "isPNaClABIExternalName" -- Subzero checked for the symbol
"__pnacl_pso_root" as a function, but it is a declaration, and should be
checked as one. Additionally, when the PNaClTranslator is verifying the linkage
of declarations, allow "__pnacl_pso_root" to be flipped to external as a special
case.
Previously, translating a pso with --use-sz caused the warning:
"cannot find entry symbol '__pnacl_pso_root'".
That warning is removed with this CL.
Fixes revert from https://codereview.chromium.org/1745783002/
TEST=external_declaration.ll
BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351
R=kschimpf@google.com, stichnot@chromium.org
Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=0a3c523d94c620457f1572ac5c526a4fee983b0e
Patch Set 1 #
Total comments: 17
Patch Set 2 : Added test #
Total comments: 6
Patch Set 3 : Updated external declaration test #
Messages
Total messages: 17 (7 generated)
Description was changed from ========== PNaCL Dynamic Linking: Force __pnacl_pso_root to be an external declaration. This CL updates "isPNaClABIExternalName" -- Subzero checked for the symbol "__pnacl_pso_root" as a function, but it is a declaration, and should be checked as one. Additionally, when the PNaClTranslator is verifying the linkage of declarations, allow "__pnacl_pso_root" to be flipped to external as a special case. Fixes revert from https://codereview.chromium.org/1745783002/ TEST=pnacl-translate -pso --use-sz -arch x86-32-nonsfi test_pll.final.pso BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351 ========== to ========== PNaCl Dynamic Linking: Force __pnacl_pso_root to be an external declaration. This CL updates "isPNaClABIExternalName" -- Subzero checked for the symbol "__pnacl_pso_root" as a function, but it is a declaration, and should be checked as one. Additionally, when the PNaClTranslator is verifying the linkage of declarations, allow "__pnacl_pso_root" to be flipped to external as a special case. Fixes revert from https://codereview.chromium.org/1745783002/ TEST=pnacl-translate -pso --use-sz -arch x86-32-nonsfi test_pll.final.pso BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351 ==========
Description was changed from ========== PNaCl Dynamic Linking: Force __pnacl_pso_root to be an external declaration. This CL updates "isPNaClABIExternalName" -- Subzero checked for the symbol "__pnacl_pso_root" as a function, but it is a declaration, and should be checked as one. Additionally, when the PNaClTranslator is verifying the linkage of declarations, allow "__pnacl_pso_root" to be flipped to external as a special case. Fixes revert from https://codereview.chromium.org/1745783002/ TEST=pnacl-translate -pso --use-sz -arch x86-32-nonsfi test_pll.final.pso BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351 ========== to ========== PNaCl Dynamic Linking: Force __pnacl_pso_root to be an external declaration. This CL updates "isPNaClABIExternalName" -- Subzero checked for the symbol "__pnacl_pso_root" as a function, but it is a declaration, and should be checked as one. Additionally, when the PNaClTranslator is verifying the linkage of declarations, allow "__pnacl_pso_root" to be flipped to external as a special case. Previously, translating a pso with --use-sz caused the warning: "cannot find entry symbol '__pnacl_pso_root'". That warning is removed with this CL. Fixes revert from https://codereview.chromium.org/1745783002/ TEST=pnacl-translate -pso --use-sz -arch x86-32-nonsfi test_pll.final.pso BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351 ==========
smklein@chromium.org changed reviewers: + jpp@chromium.org, stichnot@chromium.org
This version is passing "make -f Makefile.standalone presubmit" (conditional on the le32-nacl-objdump patch landing). Sorry about causing a need for a revert earlier!
mseaborn@chromium.org changed reviewers: + mseaborn@chromium.org
https://codereview.chromium.org/1774383002/diff/1/src/IceGlobalInits.h File src/IceGlobalInits.h (right): https://codereview.chromium.org/1774383002/diff/1/src/IceGlobalInits.h#newcode74 src/IceGlobalInits.h:74: void setLinkage(llvm::GlobalValue::LinkageTypes l) { Linkage = l; } Style nit: should be "L" https://codereview.chromium.org/1774383002/diff/1/src/IceGlobalInits.h#newcod... src/IceGlobalInits.h:152: } else if (isPNaClABIExternalName()) { Isn't this equivalent to what this function was doing before? https://codereview.chromium.org/1774383002/diff/1/src/IceGlobalInits.h#newcod... src/IceGlobalInits.h:197: static constexpr char Start[] = "_start"; Why not just write "_start" on the following line? https://codereview.chromium.org/1774383002/diff/1/src/PNaClTranslator.cpp File src/PNaClTranslator.cpp (right): https://codereview.chromium.org/1774383002/diff/1/src/PNaClTranslator.cpp#new... src/PNaClTranslator.cpp:479: if (Decl->isPNaClABIExternalName()) { As I commented on https://codereview.chromium.org/1740033002/diff/60001/src/PNaClTranslator.cpp..., you should only be doing this for GlobalVariables, not Functions. I think that means you don't need isPNaClABIExternalName() to be virtual.
https://codereview.chromium.org/1774383002/diff/1/src/IceGlobalInits.h File src/IceGlobalInits.h (right): https://codereview.chromium.org/1774383002/diff/1/src/IceGlobalInits.h#newcod... src/IceGlobalInits.h:416: const bool SameMangling = (OldSuppressMangling == getSuppressMangling()); Just FYI. The cross test framework involves mangling global identifier names with some prefix so that pnacl-llc and pnacl-sz can each translate the same file and the results can be linked together with a driver without multiply defined symbol errors. There is logic that determines during parsing whether a symbol should be mangled. The mangling happens when setName() is called. For global variables, the logic depends on whether or not the global variable has initializers, and so there is a check here that the mangling decision doesn't change after the variable name is set. There was no such check for function names, because the logic depended only on const members that were set in the ctor. However, now you are allowing the linkage to change, possibly after the name has been set and the mangling decision determined. As such, I wonder if setLinkage() should do a similar check to be safe. I don't like how fragile this is, but I couldn't find a better way without re-architecting the way this kind of parsing is done. (Actually, maybe - in a separate CL - an RAII helper could be added to every setter.) In this case, though, a cross test should never define the entry point, so this wouldn't be an issue, but if setLinkage() is exposed, future CLs might be tempted to use it "incorrectly". https://codereview.chromium.org/1774383002/diff/1/src/IceGlobalInits.h#newcod... src/IceGlobalInits.h:450: return getName() == PnaclPsoRoot; Should this be conditional on GlobalContext::getFlags().getUseNonsfi() ?
stichnot@chromium.org changed reviewers: + kschimpf@google.com
+karl who knows this code better
https://codereview.chromium.org/1774383002/diff/1/src/IceGlobalInits.h File src/IceGlobalInits.h (right): https://codereview.chromium.org/1774383002/diff/1/src/IceGlobalInits.h#newcod... src/IceGlobalInits.h:260: void dump(Ostream &Stream) const override final; Doesn't final imply override? Why are both necessary? https://codereview.chromium.org/1774383002/diff/1/src/PNaClTranslator.cpp File src/PNaClTranslator.cpp (right): https://codereview.chromium.org/1774383002/diff/1/src/PNaClTranslator.cpp#new... src/PNaClTranslator.cpp:483: Decl->setLinkage(llvm::GlobalValue::ExternalLinkage); This code is somewhat fragile. The name was earlier set when the module value symbol table was parsed. It could have gotten mangled then. By changing this flag now (rather than when the module value symbol table is processed), you risk mangling the name twice. Consider moving this code to ModuleValueSymtabParser::setValueName().
Description was changed from ========== PNaCl Dynamic Linking: Force __pnacl_pso_root to be an external declaration. This CL updates "isPNaClABIExternalName" -- Subzero checked for the symbol "__pnacl_pso_root" as a function, but it is a declaration, and should be checked as one. Additionally, when the PNaClTranslator is verifying the linkage of declarations, allow "__pnacl_pso_root" to be flipped to external as a special case. Previously, translating a pso with --use-sz caused the warning: "cannot find entry symbol '__pnacl_pso_root'". That warning is removed with this CL. Fixes revert from https://codereview.chromium.org/1745783002/ TEST=pnacl-translate -pso --use-sz -arch x86-32-nonsfi test_pll.final.pso BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351 ========== to ========== PNaCl Dynamic Linking: Force __pnacl_pso_root to be an external declaration. This CL updates "isPNaClABIExternalName" -- Subzero checked for the symbol "__pnacl_pso_root" as a function, but it is a declaration, and should be checked as one. Additionally, when the PNaClTranslator is verifying the linkage of declarations, allow "__pnacl_pso_root" to be flipped to external as a special case. Previously, translating a pso with --use-sz caused the warning: "cannot find entry symbol '__pnacl_pso_root'". That warning is removed with this CL. Fixes revert from https://codereview.chromium.org/1745783002/ TEST=external_declaration.ll BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351 ==========
Added a (very simple) external declaration test. Currently not Non-SFI specific (though it easily could be, if we decide that's better at the moment). https://codereview.chromium.org/1774383002/diff/1/src/IceGlobalInits.h File src/IceGlobalInits.h (right): https://codereview.chromium.org/1774383002/diff/1/src/IceGlobalInits.h#newcode74 src/IceGlobalInits.h:74: void setLinkage(llvm::GlobalValue::LinkageTypes l) { Linkage = l; } On 2016/03/09 03:37:15, Mark Seaborn wrote: > Style nit: should be "L" Done. https://codereview.chromium.org/1774383002/diff/1/src/IceGlobalInits.h#newcod... src/IceGlobalInits.h:152: } else if (isPNaClABIExternalName()) { On 2016/03/09 03:37:15, Mark Seaborn wrote: > Isn't this equivalent to what this function was doing before? Agreed -- sorry, changed back. https://codereview.chromium.org/1774383002/diff/1/src/IceGlobalInits.h#newcod... src/IceGlobalInits.h:197: static constexpr char Start[] = "_start"; On 2016/03/09 03:37:15, Mark Seaborn wrote: > Why not just write "_start" on the following line? Done, and also with "__pnacl_pso_root". https://codereview.chromium.org/1774383002/diff/1/src/IceGlobalInits.h#newcod... src/IceGlobalInits.h:260: void dump(Ostream &Stream) const override final; On 2016/03/09 17:06:50, Karl wrote: > Doesn't final imply override? Why are both necessary? This was causing the following clang error (hence the other 'dump' being marked override): error: 'dump' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override] But in this instance, I agree, I think it is unnecessary with 'final'. Updated. https://codereview.chromium.org/1774383002/diff/1/src/IceGlobalInits.h#newcod... src/IceGlobalInits.h:416: const bool SameMangling = (OldSuppressMangling == getSuppressMangling()); On 2016/03/09 16:29:35, stichnot wrote: > Just FYI. > > The cross test framework involves mangling global identifier names with some > prefix so that pnacl-llc and pnacl-sz can each translate the same file and the > results can be linked together with a driver without multiply defined symbol > errors. There is logic that determines during parsing whether a symbol should > be mangled. The mangling happens when setName() is called. For global > variables, the logic depends on whether or not the global variable has > initializers, and so there is a check here that the mangling decision doesn't > change after the variable name is set. > > There was no such check for function names, because the logic depended only on > const members that were set in the ctor. However, now you are allowing the > linkage to change, possibly after the name has been set and the mangling > decision determined. As such, I wonder if setLinkage() should do a similar > check to be safe. > > I don't like how fragile this is, but I couldn't find a better way without > re-architecting the way this kind of parsing is done. > > (Actually, maybe - in a separate CL - an RAII helper could be added to every > setter.) > > In this case, though, a cross test should never define the entry point, so this > wouldn't be an issue, but if setLinkage() is exposed, future CLs might be > tempted to use it "incorrectly". I'll add an assert to "setLinkage" that the name is empty. This is still a little fragile, but it's better than leaving "setLinkage" fully freely available for anyone to use. https://codereview.chromium.org/1774383002/diff/1/src/IceGlobalInits.h#newcod... src/IceGlobalInits.h:450: return getName() == PnaclPsoRoot; On 2016/03/09 16:29:35, stichnot wrote: > Should this be conditional on GlobalContext::getFlags().getUseNonsfi() ? Is there a reason dynamic linking couldn't be supported in an SFI application? https://codereview.chromium.org/1774383002/diff/1/src/PNaClTranslator.cpp File src/PNaClTranslator.cpp (right): https://codereview.chromium.org/1774383002/diff/1/src/PNaClTranslator.cpp#new... src/PNaClTranslator.cpp:479: if (Decl->isPNaClABIExternalName()) { On 2016/03/09 03:37:15, Mark Seaborn wrote: > As I commented on > https://codereview.chromium.org/1740033002/diff/60001/src/PNaClTranslator.cpp..., > you should only be doing this for GlobalVariables, not Functions. > > I think that means you don't need isPNaClABIExternalName() to be virtual. Changed so that setLinkage is only considered for VariableDeclarations. However, "isPNaClABIExternalName" is still virtual. https://codereview.chromium.org/1774383002/diff/1/src/PNaClTranslator.cpp#new... src/PNaClTranslator.cpp:483: Decl->setLinkage(llvm::GlobalValue::ExternalLinkage); On 2016/03/09 17:06:50, Karl wrote: > This code is somewhat fragile. The name was earlier set when the module value > symbol table was parsed. It could have gotten mangled then. By changing this > flag now (rather than when the module value symbol table is processed), you risk > mangling the name twice. Consider moving this code to > ModuleValueSymtabParser::setValueName(). Code moved to "ModuleValueSymtabParser::setValueName()", and updated "setLinkage" to only operate on Declarations with no name. "isPnaClAbiExternalName" updated so that a string can be passed as an argument, even when the declaration itself may not yet have a name.
Otherwise LGTM, but please get Karl's LGTM as well. https://codereview.chromium.org/1774383002/diff/1/src/IceGlobalInits.h File src/IceGlobalInits.h (right): https://codereview.chromium.org/1774383002/diff/1/src/IceGlobalInits.h#newcod... src/IceGlobalInits.h:450: return getName() == PnaclPsoRoot; On 2016/03/10 23:45:22, Sean Klein wrote: > On 2016/03/09 16:29:35, stichnot wrote: > > Should this be conditional on GlobalContext::getFlags().getUseNonsfi() ? > > Is there a reason dynamic linking couldn't be supported in an SFI application? Sorry, I got confused about the scope of this change. I was thinking it was for Non-SFI, but that's orthgonal to PSOs. https://codereview.chromium.org/1774383002/diff/20001/tests_lit/llvm2ice_test... File tests_lit/llvm2ice_tests/external_declaration.ll (right): https://codereview.chromium.org/1774383002/diff/20001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/external_declaration.ll:1: ; RUN: %if --need=allow_dump --command %p2i -i %s --target=x8632 \ 1. It would be nice to add a brief comment at the top describing the purpose of the test file. Even if not all the other .ll tests do this... 2. It would probably be cleaner to remove the pnacl-sz flags that don't really matter: --target and -O2. https://codereview.chromium.org/1774383002/diff/20001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/external_declaration.ll:2: ; RUN --filetype=asm --args -O2 -nonsfi=0 \ s/RUN/RUN:/ otherwise these options are just ignored :) Probably the default filetype is iasm and default nonsfi is 0, so this works accidentally. https://codereview.chromium.org/1774383002/diff/20001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/external_declaration.ll:5: ; Verify that "__pnacl_pso_root", a specially named symbol, is made reflow comment to 80-col
https://codereview.chromium.org/1774383002/diff/20001/tests_lit/llvm2ice_test... File tests_lit/llvm2ice_tests/external_declaration.ll (right): https://codereview.chromium.org/1774383002/diff/20001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/external_declaration.ll:1: ; RUN: %if --need=allow_dump --command %p2i -i %s --target=x8632 \ On 2016/03/11 01:20:29, stichnot wrote: > 1. It would be nice to add a brief comment at the top describing the purpose of > the test file. Even if not all the other .ll tests do this... > > 2. It would probably be cleaner to remove the pnacl-sz flags that don't really > matter: --target and -O2. Done. https://codereview.chromium.org/1774383002/diff/20001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/external_declaration.ll:2: ; RUN --filetype=asm --args -O2 -nonsfi=0 \ On 2016/03/11 01:20:29, stichnot wrote: > s/RUN/RUN:/ > otherwise these options are just ignored :) > > Probably the default filetype is iasm and default nonsfi is 0, so this works > accidentally. Yikes, my bad. Fixed. https://codereview.chromium.org/1774383002/diff/20001/tests_lit/llvm2ice_test... tests_lit/llvm2ice_tests/external_declaration.ll:5: ; Verify that "__pnacl_pso_root", a specially named symbol, is made On 2016/03/11 01:20:29, stichnot wrote: > reflow comment to 80-col Done.
lgtm
Description was changed from ========== PNaCl Dynamic Linking: Force __pnacl_pso_root to be an external declaration. This CL updates "isPNaClABIExternalName" -- Subzero checked for the symbol "__pnacl_pso_root" as a function, but it is a declaration, and should be checked as one. Additionally, when the PNaClTranslator is verifying the linkage of declarations, allow "__pnacl_pso_root" to be flipped to external as a special case. Previously, translating a pso with --use-sz caused the warning: "cannot find entry symbol '__pnacl_pso_root'". That warning is removed with this CL. Fixes revert from https://codereview.chromium.org/1745783002/ TEST=external_declaration.ll BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351 ========== to ========== PNaCl Dynamic Linking: Force __pnacl_pso_root to be an external declaration. This CL updates "isPNaClABIExternalName" -- Subzero checked for the symbol "__pnacl_pso_root" as a function, but it is a declaration, and should be checked as one. Additionally, when the PNaClTranslator is verifying the linkage of declarations, allow "__pnacl_pso_root" to be flipped to external as a special case. Previously, translating a pso with --use-sz caused the warning: "cannot find entry symbol '__pnacl_pso_root'". That warning is removed with this CL. Fixes revert from https://codereview.chromium.org/1745783002/ TEST=external_declaration.ll BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4351 R=kschimpf@google.com, stichnot@chromium.org Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) manually as 0a3c523d94c620457f1572ac5c526a4fee983b0e (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
