[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

This commit caused following regression

https://github.com/llvm/llvm-project/issues/57449#issuecomment-1232102039


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

The ScudoCUnitTest-mips64el-Test failure looks like a mips64el specific issue.

  FAILED: lib/scudo/standalone/tests/ScudoCUnitTest-mips64el-Test 
  cd 
/b/sanitizer-x86_64-linux-qemu/build/llvm_build2_debug_mips64el_qemu/lib/scudo/standalone/tests
 && /b/sanitizer-x86_64-linux-qemu/build/llvm_build0/bin/clang++ 
ScudoUnitTestsObjects.wrappers_c_test.cpp.mips64el.o 
ScudoUnitTestsObjects.scudo_unit_test_main.cpp.mips64el.o 
ScudoUnitTestsObjects.gtest-all.cc.mips64el.o 
/b/sanitizer-x86_64-linux-qemu/build/llvm_build2_debug_mips64el_qemu/lib/scudo/standalone/tests/libRTScudoCUnitTest.mips64el.a
 -o 
/b/sanitizer-x86_64-linux-qemu/build/llvm_build2_debug_mips64el_qemu/lib/scudo/standalone/tests/./ScudoCUnitTest-mips64el-Test
 -fuse-ld=lld --target=mips64el-linux-gnuabi64 -Wthread-safety 
-Wthread-safety-reference -Wthread-safety-beta -lstdc++ -pthread -latomic
  ld.lld: error: cannot preempt symbol: DW.ref.__gxx_personality_v0
  >>> defined in ScudoUnitTestsObjects.wrappers_c_test.cpp.mips64el.o
  >>> referenced by wrappers_c_test.cpp
  >>>   
ScudoUnitTestsObjects.wrappers_c_test.cpp.mips64el.o:(.eh_frame+0x1420B)



In D120305#3436154 , @thakis wrote:

> We're seeing the same problem here 
> https://bugs.chromium.org/p/chromium/issues/detail?id=1314297=linux_upload_clang=2
>
> Reverted in e22a60b1c898a760a73417fa225c2fbe0609a69f 
>  for now.

This was a different problem: on some old systems glibc crt1.o was not compiled 
with -fPIC and cannot be linked as `-pie`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

We're seeing the same problem here 
https://bugs.chromium.org/p/chromium/issues/detail?id=1314297=linux_upload_clang=2

Reverted in e22a60b1c898a760a73417fa225c2fbe0609a69f 
 for now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

Looks like this causes a relocation failure on this buildbot: 
https://lab.llvm.org/buildbot/#/builders/169/builds/7311

  ld.lld: error: relocation R_MIPS_64 cannot be used against local symbol; 
recompile with -fPIC
  >>> defined in ScudoUnitTestsObjects.wrappers_cpp_test.cpp.mips64.o
  >>> referenced by wrappers_cpp_test.cpp
  >>>   
ScudoUnitTestsObjects.wrappers_cpp_test.cpp.mips64.o:(.eh_frame+0x15A39)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.
Herald added a subscriber: StephenFan.

@nemanjai mentioned "All the issues I was seeing seem to have been resolved. I 
can build with CLANG_DEFAULT_PIE_ON_LINUX=on without any failing sanitizer or 
crt failures."
Will recommit. And ppc64 bots have hopefully got better with the upgrade. 
Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Tom Stellard via Phabricator via cfe-commits]

tstellar added a comment.

@xbolva00 I've removed your comment due to inappropriate language and tone.  
Please find a more constructive way to communicate your point.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.
Herald added a project: All.

In D120305#3347191 , @MaskRay wrote:

> In D120305#3347177 , @nikic wrote:
>
>> Yes, because you reverted the change for that one buildbot, of course it is 
>> green now. You could have also made the buildbot green by disabling tests on 
>> that bot. Or disabling sanitizers on it. Doesn't change the fact that the 
>> configuration it was originally testing is still broken, you just hid the 
>> failure.
>
>
>
> In D120305#3347184 , @tstellar 
> wrote:
>
>> In D120305#3347161 , @MaskRay 
>> wrote:
>>
>>> In D120305#3347160 , @nikic wrote:
>>>
 @MaskRay Please revert the change and all dependent changes you have made. 
 A revert is not a personal affront to you. It's not a judgement that you 
 or your change are bad. It's a simple matter of policy and standard 
 procedure. There's a good chance that next week you'll get confirmation 
 that it's indeed some outdated libraries on the buildbot, and the change 
 can reland without any changes on your side. Or maybe it turns out that 
 this default is not quite viable for powerpc targets yet. Who knows.

 Don't worry about the churn. These short-term reverts happen all the time, 
 we're used to it. Especially for tiny changes like this one, it's really 
 no problem (reverts can be more annoying if the commit touches 1500 test 
 files).
>>>
>>> I have mentioned that https://lab.llvm.org/buildbot/#/builders/57 has been 
>>> green.
>>
>> Disabling the failing tests with an unreviewed patch is not the right way to 
>> fix this.
>
> clang-ppc64le-rhel got `-DCLANG_DEFAULT_PIE_ON_LINUX=OFF` ~9 hours ago. If 
> any of you can make the configuration live on the bot, it will work and we 
> can re-enable the tests.

https://lab.llvm.org/buildbot/#/builders/57

Somebody (@tstellar) should remove these powerpc shitty bots, red almost all 
the time as bot maintainers do nothing. There should be clear policy when to 
remove bad, broken, unmaintained bots.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

I've been looking into the compile-time regressions.

The large regression on sqlite3 is due to differences in inlining behavior. The 
inlining cost model is slightly different for PIC builds, because GEPs that are 
based on a global and have a variable offset are no longer considered free. In 
the sqlite3 case this leads to different inlining decisions, which ultimately 
lead to the large compile-time regression. I don't think there is anything 
immediately actionable here, it's just bad luck that the different cost 
modeling happens to push us across an arbitrary threshold.

I've also looked at many smaller regressions (around ~5% on individual files), 
where the common factor is that we spend a lot more time in RAGreedy (e.g. from 
2% of total compile-time up to 6%). Most of the additional cost is in the 
calculation of region splitting costs. Something is clearly very wrong here, 
because a single RAGreedy::trySplit() call shouldn't take 3M instructions to 
execute.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Tom Stellard via Phabricator via cfe-commits]

tstellar added a comment.

I have reverted the buildbot configuration change  as well: 
https://github.com/llvm/llvm-zorg/commit/c01d0787c4a11e40d0fed81b0df6841bebc16f7f

Now that everything is back to the original state, we can discuss how to 
proceed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

This patch also caused https://github.com/llvm/llvm-project/issues/54082. I 
noticed due to the revert making the openmp-offload-cuda-project 
 bot green again 
(https://lab.llvm.org/staging/#/builders/155/builds/2482)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D120305#3344938 , @nikic wrote:

> Hm, it looks like enabling PIE has a pretty big negative compile-time impact, 
> with a 20% regression on sqlite3: 
> http://llvm-compile-time-tracker.com/compare.php?from=611122892e6d5813444bdd0e1fbe0a96f6e09779=3c4ed02698afec021c6bca80740d1e58e3ee019e=instructions
>  Code-size on kimwitu++ regresses by 13%.
>
> Is that kind of impact expected?

Many groups build sqlite3 with -fPIC. -fPIC and -fPIE have similar compile time.
The metric on llvm-compile-time-tracker.com is related to 
`llvm-test-suite/CTMark/sqlite3`, which just focuses on (without the CMake 
patch) `-fno-pic` performance.

(
I checked run time, no big difference.

  % hyperfine --warmup 2 --min-runs 16 "/tmp/c/sqlite3.nopie -init sqlite3rc 
:memory: < commands" 
  Benchmark 1: /tmp/c/sqlite3.nopie -init sqlite3rc :memory: < commands
Time (mean ± σ):  2.068 s ±  0.011 s[User: 2.044 s, System: 0.024 s]
Range (min … max):2.044 s …  2.085 s16 runs
  
  % hyperfine --warmup 2 --min-runs 16 "/tmp/c/sqlite3.pie -init sqlite3rc 
:memory: < commands"  
  Benchmark 1: /tmp/c/sqlite3.pie -init sqlite3rc :memory: < commands
Time (mean ± σ):  2.053 s ±  0.015 s[User: 2.034 s, System: 0.018 s]
Range (min … max):2.027 s …  2.080 s16 runs

)

About compile time,

I run `=time -f "CPU: %Us\tReal: %es\tRAM: %MKB" /tmp/out/custom1/bin/clang 
-DNDEBUG -O3 -DNDEBUG -w -Werror=date-time -DSTDC_HEADERS=1 
-DHAVE_SYS_TYPES_H=1 -DHAVE_SYS_STAT_H=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 
-DHAVE_MEMORY_H=1 -DHAVE_STRINGS_H=1 -DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 
-DHAVE_UNISTD_H=1 -DSQLITE_OMIT_LOAD_EXTENSION=1 -DSQLITE_THREADSAFE=0 -I. -o 
CTMark/sqlite3/CMakeFiles/sqlite3.dir/sqlite3.c.o -c 
~/Dev/llvm-test-suite/CTMark/sqlite3/sqlite3.c -ftime-report` with 
-fno-pie/-fpie/-fpic to get some insight.

The IR has minor difference but pie's is slightly larger:

  % wc -l nopie.ll pie.ll
157975 nopie.ll
159675 pie.ll
  ...

If someone wants to investigate, `sqlite3Parser` changes a lot from -fno-pic to 
-fPIE, but that looks organic changes to me.
-ftime-report comparison suggests that every pass is slightly slower with 
-fPIE. ModuleInlinerWrapperPass and DevirtSCCRepeatedPass contribute most of 
the slowness.

  -fno-pic -ftime-report
 ---User Time---   --System Time--   --User+System--   ---Wall Time---  --- 
Name ---
 5.1753 ( 28.1%)   0.2359 ( 27.5%)   5.4112 ( 28.0%)   5.4123 ( 28.1%)  
ModuleInlinerWrapperPass
 5.1447 ( 27.9%)   0.2309 ( 26.9%)   5.3755 ( 27.8%)   5.3767 ( 27.9%)  
DevirtSCCRepeatedPass
 1.8716 ( 10.1%)   0.0936 ( 10.9%)   1.9652 ( 10.2%)   1.9613 ( 10.2%)  
InstCombinePass
 0.7674 (  4.2%)   0.0254 (  3.0%)   0.7928 (  4.1%)   0.7921 (  4.1%)  
GVNPass
 0.4563 (  2.5%)   0.0288 (  3.4%)   0.4851 (  2.5%)   0.4844 (  2.5%)  
InlinerPass
 0.4194 (  2.3%)   0.0174 (  2.0%)   0.4368 (  2.3%)   0.4359 (  2.3%)  
MemorySSAAnalysis
 0.3399 (  1.8%)   0.0191 (  2.2%)   0.3589 (  1.9%)   0.3578 (  1.9%)  
SimplifyCFGPass
 0.3206 (  1.7%)   0.0143 (  1.7%)   0.3349 (  1.7%)   0.3354 (  1.7%)  
BlockFrequencyAnalysis
 0.2985 (  1.6%)   0.0081 (  0.9%)   0.3065 (  1.6%)   0.3061 (  1.6%)  
CorrelatedValuePropagationPass
 0.2608 (  1.4%)   0.0158 (  1.8%)   0.2767 (  1.4%)   0.2763 (  1.4%)  
EarlyCSEPass



  -fpie -ftime-report
 ---User Time---   --System Time--   --User+System--   ---Wall Time---  --- 
Name ---
 7.8109 ( 29.2%)   0.2239 ( 26.8%)   8.0347 ( 29.1%)   8.0363 ( 29.2%)  
ModuleInlinerWrapperPass
 7.7752 ( 29.1%)   0.2230 ( 26.6%)   7.9981 ( 29.0%)   7.9997 ( 29.0%)  
DevirtSCCRepeatedPass
 2.5352 (  9.5%)   0.0807 (  9.6%)   2.6158 (  9.5%)   2.6114 (  9.5%)  
InstCombinePass
 1.2792 (  4.8%)   0.0162 (  1.9%)   1.2954 (  4.7%)   1.2948 (  4.7%)  
GVNPass
 0.6529 (  2.4%)   0.0157 (  1.9%)   0.6686 (  2.4%)   0.6674 (  2.4%)  
MemorySSAAnalysis
 0.6048 (  2.3%)   0.0261 (  3.1%)   0.6309 (  2.3%)   0.6306 (  2.3%)  
InlinerPass
 0.4625 (  1.7%)   0.0126 (  1.5%)   0.4751 (  1.7%)   0.4743 (  1.7%)  
CorrelatedValuePropagationPass
 0.4541 (  1.7%)   0.0160 (  1.9%)   0.4701 (  1.7%)   0.4690 (  1.7%)  
SimplifyCFGPass
 0.3981 (  1.5%)   0.0124 (  1.5%)   0.4105 (  1.5%)   0.4101 (  1.5%)  
EarlyCSEPass



- -fno-pie: Time (mean ± σ): 11.999 s ±  0.043 s
- -fpie: Time (mean ± σ): 14.643 s ±  0.073 s
- -fno-pie -flegacy-pass-manager: Time (mean ± σ): 16.831 s ±  0.027 s
- -fpie -flegacy-pass-manager: Time (mean ± σ): 16.887 s ±  0.099 s


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D120305#3347194 , @MaskRay wrote:

> In D120305#3347193 , @nikic wrote:
>
>> In D120305#3347192 , @tstellar 
>> wrote:
>>
>>> In D120305#3347177 , @nikic wrote:
>>>
 Yes, because you reverted the change for that one buildbot, of course it 
 is green now. You could have also made the buildbot green by disabling 
 tests on that bot. Or disabling sanitizers on it. Doesn't change the fact 
 that the configuration it was originally testing is still broken, you just 
 hid the failure.
>>>
>>> The build is green because of this commit: 
>>> https://github.com/llvm/llvm-project/commit/274ec425dcc3e3f637dd006c5e9ae33bd0e2e917
>>>   The buildbot change you are referring to, which is 
>>> https://github.com/llvm/llvm-zorg/commit/b6ddf02ce3a54da2df29e7e599b1838167e0e3ad,
>>>  has not taken effect yet, because the buildbot server has not been 
>>> restarted.
>>
>> Wow, that's even worse. So now it's not just a change to that one buildbot, 
>> but sanitizer tests were disabled for powerpc entirely?!
>
> Only few sanitizer_common and lsan tests, not entirely. It should be 
> re-enabled pretty soon once the llvm-zorg change is made live.
> That was I mentioned that "we can enable the tests".
>
> I think at this point, if you prefer, I am happy to revert this change the 
> disabling (it needs quite a bit of tests), so that we can know whether 
> -fno-pic and -fpie have a large difference on sqlite3 performance.
>
> I lost my control when I saw https://reviews.llvm.org/D120305#3347058 and 
> Tom's first reply to it.
> For quite few hours yesterday, I did not know my fixed did not fix the 
> problem or that nobody tried making llvm-zorg change work.

@nikic If you still want to revert, you can use https://reviews.llvm.org/P8281
I have done some testing that it is fine.

It is too late here so I'll not be responsive for many hours.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D120305#3347193 , @nikic wrote:

> In D120305#3347192 , @tstellar 
> wrote:
>
>> In D120305#3347177 , @nikic wrote:
>>
>>> Yes, because you reverted the change for that one buildbot, of course it is 
>>> green now. You could have also made the buildbot green by disabling tests 
>>> on that bot. Or disabling sanitizers on it. Doesn't change the fact that 
>>> the configuration it was originally testing is still broken, you just hid 
>>> the failure.
>>
>> The build is green because of this commit: 
>> https://github.com/llvm/llvm-project/commit/274ec425dcc3e3f637dd006c5e9ae33bd0e2e917
>>   The buildbot change you are referring to, which is 
>> https://github.com/llvm/llvm-zorg/commit/b6ddf02ce3a54da2df29e7e599b1838167e0e3ad,
>>  has not taken effect yet, because the buildbot server has not been 
>> restarted.
>
> Wow, that's even worse. So now it's not just a change to that one buildbot, 
> but sanitizer tests were disabled for powerpc entirely?!

Only few sanitizer_common and lsan tests, not entirely. It should be re-enabled 
pretty soon once the llvm-zorg change is made live.
That was I mentioned that "we can enable the tests".

I think at this point, if you prefer, I am happy to revert this change the 
disabling (it needs quite a bit of tests), so that we can know whether -fno-pic 
and -fpie have a large difference on sqlite3 performance.

I lost my control when I saw https://reviews.llvm.org/D120305#3347058 and Tom's 
first reply to it.
For quite few hours yesterday, I did not know my fixed did not fix the problem 
or that nobody tried making llvm-zorg change work.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

In D120305#3347192 , @tstellar wrote:

> In D120305#3347177 , @nikic wrote:
>
>> Yes, because you reverted the change for that one buildbot, of course it is 
>> green now. You could have also made the buildbot green by disabling tests on 
>> that bot. Or disabling sanitizers on it. Doesn't change the fact that the 
>> configuration it was originally testing is still broken, you just hid the 
>> failure.
>
> The build is green because of this commit: 
> https://github.com/llvm/llvm-project/commit/274ec425dcc3e3f637dd006c5e9ae33bd0e2e917
>   The buildbot change you are referring to, which is 
> https://github.com/llvm/llvm-zorg/commit/b6ddf02ce3a54da2df29e7e599b1838167e0e3ad,
>  has not taken effect yet, because the buildbot server has not been restarted.

Wow, that's even worse. So now it's not just a change to that one buildbot, but 
sanitizer tests were disabled for powerpc entirely?!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Tom Stellard via Phabricator via cfe-commits]

tstellar added a comment.

In D120305#3347177 , @nikic wrote:

> Yes, because you reverted the change for that one buildbot, of course it is 
> green now. You could have also made the buildbot green by disabling tests on 
> that bot. Or disabling sanitizers on it. Doesn't change the fact that the 
> configuration it was originally testing is still broken, you just hid the 
> failure.

The build is green because of this commit: 
https://github.com/llvm/llvm-project/commit/274ec425dcc3e3f637dd006c5e9ae33bd0e2e917
  The buildbot change you are referring to, which is 
https://github.com/llvm/llvm-zorg/commit/b6ddf02ce3a54da2df29e7e599b1838167e0e3ad,
 has not taken effect yet, because the buildbot server has not been restarted.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D120305#3347177 , @nikic wrote:

> Yes, because you reverted the change for that one buildbot, of course it is 
> green now. You could have also made the buildbot green by disabling tests on 
> that bot. Or disabling sanitizers on it. Doesn't change the fact that the 
> configuration it was originally testing is still broken, you just hid the 
> failure.



In D120305#3347184 , @tstellar wrote:

> In D120305#3347161 , @MaskRay wrote:
>
>> In D120305#3347160 , @nikic wrote:
>>
>>> @MaskRay Please revert the change and all dependent changes you have made. 
>>> A revert is not a personal affront to you. It's not a judgement that you or 
>>> your change are bad. It's a simple matter of policy and standard procedure. 
>>> There's a good chance that next week you'll get confirmation that it's 
>>> indeed some outdated libraries on the buildbot, and the change can reland 
>>> without any changes on your side. Or maybe it turns out that this default 
>>> is not quite viable for powerpc targets yet. Who knows.
>>>
>>> Don't worry about the churn. These short-term reverts happen all the time, 
>>> we're used to it. Especially for tiny changes like this one, it's really no 
>>> problem (reverts can be more annoying if the commit touches 1500 test 
>>> files).
>>
>> I have mentioned that https://lab.llvm.org/buildbot/#/builders/57 has been 
>> green.
>
> Disabling the failing tests with an unreviewed patch is not the right way to 
> fix this.

clang-ppc64le-rhel got `-DCLANG_DEFAULT_PIE_ON_LINUX=OFF` ~9 hours ago. If any 
of you can make the configuration live on the bot, it will work and we can 
re-enable the tests.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Tom Stellard via Phabricator via cfe-commits]

tstellar added a comment.

In D120305#3347161 , @MaskRay wrote:

> In D120305#3347160 , @nikic wrote:
>
>> @MaskRay Please revert the change and all dependent changes you have made. A 
>> revert is not a personal affront to you. It's not a judgement that you or 
>> your change are bad. It's a simple matter of policy and standard procedure. 
>> There's a good chance that next week you'll get confirmation that it's 
>> indeed some outdated libraries on the buildbot, and the change can reland 
>> without any changes on your side. Or maybe it turns out that this default is 
>> not quite viable for powerpc targets yet. Who knows.
>>
>> Don't worry about the churn. These short-term reverts happen all the time, 
>> we're used to it. Especially for tiny changes like this one, it's really no 
>> problem (reverts can be more annoying if the commit touches 1500 test files).
>
> I have mentioned that https://lab.llvm.org/buildbot/#/builders/57 has been 
> green.

Disabling the failing tests with an unreviewed patch is not the right way to 
fix this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

Yes, because you reverted the change for that one buildbot, of course it is 
green now. You could have also made the buildbot green by disabling tests on 
that bot. Or disabling sanitizers on it. Doesn't change the fact that the 
configuration it was originally testing is still broken, you just hid the 
failure.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D120305#3347150 , @tstellar wrote:

> [...]
>
>>> It's not acceptable, because buildbots are meant to represent a specific 
>>> configuration that the buildbot owner cares about.  Changing the buildbot 
>>> configuration makes the buildbot no longer useful to them since it is now 
>>> testing something different.  For example, if someone is running production 
>>> builds that used the same configuration as the buildbot, those production 
>>> builds are still broken even though the buildbot is green.  Configuration 
>>> changes like this need to have approval of the bot owner.
>>
>> If clang-ppc64le-rhel works with `-DCLANG_DEFAULT_PIE_ON_LINUX=off` but not 
>> with  `-DCLANG_DEFAULT_PIE_ON_LINUX=on`, adding  
>> `-DCLANG_DEFAULT_PIE_ON_LINUX=off` makes the intention explicit. I am not 
>> sure why this isn't acceptable.
>> It's a tech debt, though, as we are making configurations fragmented.
>>
>>> I still don't understand why you can't revert the patch.  I've encountered 
>>> this same situation numerous times while working on this project and no one 
>>> has ever objected this much to doing a revert.  The fact that this is the 
>>> second time this has happened is concerning to me.
>>
>> I have stated my reasoning. Configuration churn can also be a problem to 
>> users.
>> Consider what if the DWARF v5 patch got reverted and relanded back and 
>> forth. Downstream users would keep observing changing behaviors.
>>
>> In this case, really even normal ppc64le machines (including some bots) were 
>> happy and just that one was picky.
>> Given that the bot does not have a high success rate (track record) for at 
>> least the past month, I am unsure I am supposed to revert my change.
>>
>> It is more concerning to me that a bot maintainer leaves an unstable bot for 
>> so long and is not even willing to spend very little time to make a 
>> llvm-zorg change live (perhaps just restart the bot software), but rather is 
>> more willing to **write such a long reply taking it very personally**.
>> (Sorry, I know when I write this, I was a bit in a mood. I felt quite 
>> frustrated at this point, so I could not keep using the tune when I made the 
>> reply https://reviews.llvm.org/D120305#3347094)
>>
>> I can response to you that I have shared the thread to some folks and at 
>> least two agree with me that nemanjai's reply made the discussion less 
>> technical but more personal.
>
> Can these folks provide their feedback on the thread?

Folks willing to provide feedback usually want to be anonymous. You may 
understand that they don't want to be bothered by further messages and don't 
want to stand by one side.

I know my replies are wasting subscribers' time and I feel sorry about that.
But I can assure to you that if without 
https://reviews.llvm.org/D120305#3347058 and your reply to that comment, we 
were likely in a green state pretty early 拾


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D120305#3347160 , @nikic wrote:

> @MaskRay Please revert the change and all dependent changes you have made. A 
> revert is not a personal affront to you. It's not a judgement that you or 
> your change are bad. It's a simple matter of policy and standard procedure. 
> There's a good chance that next week you'll get confirmation that it's indeed 
> some outdated libraries on the buildbot, and the change can reland without 
> any changes on your side. Or maybe it turns out that this default is not 
> quite viable for powerpc targets yet. Who knows.
>
> Don't worry about the churn. These short-term reverts happen all the time, 
> we're used to it. Especially for tiny changes like this one, it's really no 
> problem (reverts can be more annoying if the commit touches 1500 test files).

I have mentioned that https://lab.llvm.org/buildbot/#/builders/57 has been 
green.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

@MaskRay Please revert the change and all dependent changes you have made. A 
revert is not a personal affront to you. It's not a judgement that you or your 
change are bad. It's a simple matter of policy and standard procedure. There's 
a good chance that next week you'll get confirmation that it's indeed some 
outdated libraries on the buildbot, and the change can reland without any 
changes on your side. Or maybe it turns out that this default is not quite 
viable for powerpc targets yet. Who knows.

Don't worry about the churn. These short-term reverts happen all the time, 
we're used to it. Especially for tiny changes like this one, it's really no 
problem (reverts can be more annoying if the commit touches 1500 test files).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D120305#3347152 , @tstellar wrote:

> In D120305#3347151 , @xbolva00 
> wrote:
>
 Given that the bot does not have a high success rate (track record) for at 
 least the past month, I am unsure I am supposed to revert my change.
>>
>> What is interesting that always the patch author is the person to be blamed 
>> even when there are situations where it is so obvious that a bot has a 
>> problem. LLVM project/board should also care about buildbots and whether 
>> they work - also there should be strict policy about bot removal if 
>> maintainer does not want to spend any time to fix problems.
>>
>> It is very annoying to have unstable buildbots. One one side LLVM tries to 
>> be very inclusive and open for new people, on the other hand LLVM scares 
>> them with broken bots - first experience could be very bad.
>
> The problem here is not an unstable bot.  If you look at the build log for 
> the bot, it is very clear that this patch introduced the failures.



- https://lab.llvm.org/buildbot/#/builders/57?numbuilds=1000 
(clang-ppc64le-rhel) the bot broken by this PIE change
- https://lab.llvm.org/buildbot/#/builders/19?numbuilds=1000 
(sanitizer-ppc64le-linux), happy with this change
- https://lab.llvm.org/buildbot/#/builders/18?numbuilds=1000 
(sanitizer-ppc64be-linux), happy with this change
- https://lab.llvm.org/buildbot/#/builders/121?numbuilds=1000 
(clang-ppc64le-linux-multistage), happy with this change

clang-ppc64le-rhel has the lowest success rate.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Tom Stellard via Phabricator via cfe-commits]

tstellar added a comment.

In D120305#3347151 , @xbolva00 wrote:

>>> Given that the bot does not have a high success rate (track record) for at 
>>> least the past month, I am unsure I am supposed to revert my change.
>
> What is interesting that always the patch author is the person to be blamed 
> even when there are situations where it is so obvious that a bot has a 
> problem. LLVM project/board should also care about buildbots and whether they 
> work - also there should be strict policy about bot removal if maintainer 
> does not want to spend any time to fix problems.



> It is very annoying to have unstable buildbots. One one side LLVM tries to be 
> very inclusive and open for new people, on the other hand LLVM scares them 
> with broken bots - first experience could be very bad.



In D120305#3347151 , @xbolva00 wrote:

>>> Given that the bot does not have a high success rate (track record) for at 
>>> least the past month, I am unsure I am supposed to revert my change.
>
> What is interesting that always the patch author is the person to be blamed 
> even when there are situations where it is so obvious that a bot has a 
> problem. LLVM project/board should also care about buildbots and whether they 
> work - also there should be strict policy about bot removal if maintainer 
> does not want to spend any time to fix problems.
>
> It is very annoying to have unstable buildbots. One one side LLVM tries to be 
> very inclusive and open for new people, on the other hand LLVM scares them 
> with broken bots - first experience could be very bad.

The problem here is not an unstable bot.  If you look at the build log for the 
bot, it is very clear that this patch introduced the failures.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

>> Given that the bot does not have a high success rate (track record) for at 
>> least the past month, I am unsure I am supposed to revert my change.

What is interesting that always the patch author is the person to be blamed 
even when there are situations where it is so obvious that a bot has a problem. 
LLVM project/board should also care about buildbots and whether they work - 
also there should be strict policy about bot removal if maintainer does not 
want to spend any time to fix problems.

It is very annoying to have unstable buildbots. One one side LLVM tries to be 
very inclusive and open for new people, on the other hand LLVM scares them with 
broken bots - first experience could be very bad.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Tom Stellard via Phabricator via cfe-commits]

tstellar added a comment.

In D120305#3347148 , @MaskRay wrote:

> In D120305#3347145 , @tstellar 
> wrote:
>
>> In D120305#3347143 , @MaskRay 
>> wrote:
>>
>>> In D120305#3347142 , @tstellar 
>>> wrote:
>>>
 In D120305#3347139 , @MaskRay 
 wrote:

> In D120305#3347109 , @tstellar 
> wrote:
>
>> [...]
>> The issue here has nothing to do with the technical merits of the patch 
>> or what the root cause of the problem is.  The policy for this project 
>> is that if you commit a patch that breaks someone's configuration 
>> (especially a buildbot), then it needs to be fixed quickly or reverted.  
>> I get that this policy can be frustrating as a committer when you feel 
>> your patch is correct, and the real problem is elsewhere, but this is 
>> still the policy and it should be followed.
>
> 7 hours ago my 
> https://github.com/llvm/llvm-zorg/commit/b6ddf02ce3a54da2df29e7e599b1838167e0e3ad
>  was sufficient to fix the issue and was the suggested fix in my opinion.
> Unfortunately nobody on the PowerPC side made the change effective in the 
> build bot. Rather, I received such a heated message 
> (https://reviews.llvm.org/D120305#3347058).
> It was another way to fix the redness (revert) but IMO not justified.

 I feel like we are talking past each other at this point, but in general 
 changing the buildbot configuration is not an acceptable solution to a 
 broken bot.  Did the bot owner approve that change?
>>>
>>> Unsure why it isn't acceptable. There is fairly strong evidence that that 
>>> specific ppc64le buildbot has a stability issue and 
>>> `-DCLANG_DEFAULT_PIE_ON_LINUX=OFF` keeps it as if before this CMake patch.
>>> We can always discuss this with @nemanjai in another place, e.g. in IRC if 
>>> we want to stop bothering others subscribing to this thread.
>>
>> It's not acceptable, because buildbots are meant to represent a specific 
>> configuration that the buildbot owner cares about.  Changing the buildbot 
>> configuration makes the buildbot no longer useful to them since it is now 
>> testing something different.  For example, if someone is running production 
>> builds that used the same configuration as the buildbot, those production 
>> builds are still broken even though the buildbot is green.  Configuration 
>> changes like this need to have approval of the bot owner.
>
> If clang-ppc64le-rhel works with `-DCLANG_DEFAULT_PIE_ON_LINUX=off` but not 
> with  `-DCLANG_DEFAULT_PIE_ON_LINUX=on`, adding  
> `-DCLANG_DEFAULT_PIE_ON_LINUX=off` makes the intention explicit. I am not 
> sure why this isn't acceptable.
> It's a tech debt, though, as we are making configurations fragmented.
>
>> I still don't understand why you can't revert the patch.  I've encountered 
>> this same situation numerous times while working on this project and no one 
>> has ever objected this much to doing a revert.  The fact that this is the 
>> second time this has happened is concerning to me.
>
> I have stated my reasoning. Configuration churn can also be a problem to 
> users.
> Consider what if the DWARF v5 patch got reverted and relanded back and forth. 
> Downstream users would keep observing changing behaviors.
>
> In this case, really even normal ppc64le machines (including some bots) were 
> happy and just that one was picky.
> Given that the bot does not have a high success rate (track record) for at 
> least the past month, I am unsure I am supposed to revert my change.
>
> It is more concerning to me that a bot maintainer leaves an unstable bot for 
> so long and is not even willing to spend very little time to make a llvm-zorg 
> change live (perhaps just restart the bot software), but rather is more 
> willing to **write such a long reply taking it very personally**.
> (Sorry, I know when I write this, I was a bit in a mood. I felt quite 
> frustrated at this point, so I could not keep using the tune when I made the 
> reply https://reviews.llvm.org/D120305#3347094)
>
> I can response to you that I have shared the thread to some folks and at 
> least two agree with me that nemanjai's reply made the discussion less 
> technical but more personal.

Can these folks provide their feedback on the thread?

> And one pointed out that your "nemanjai is correct" message was made a bit 
> arbitrary, without evidence of considering the full context.

The Reversion Policy is documented here: 
https://llvm.org/docs/DeveloperPolicy.html#patch-reversion-policy


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing 

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

https://lab.llvm.org/buildbot/#/builders/57 is green


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D120305#3347145 , @tstellar wrote:

> In D120305#3347143 , @MaskRay wrote:
>
>> In D120305#3347142 , @tstellar 
>> wrote:
>>
>>> In D120305#3347139 , @MaskRay 
>>> wrote:
>>>
 In D120305#3347109 , @tstellar 
 wrote:

> [...]
> The issue here has nothing to do with the technical merits of the patch 
> or what the root cause of the problem is.  The policy for this project is 
> that if you commit a patch that breaks someone's configuration 
> (especially a buildbot), then it needs to be fixed quickly or reverted.  
> I get that this policy can be frustrating as a committer when you feel 
> your patch is correct, and the real problem is elsewhere, but this is 
> still the policy and it should be followed.

 7 hours ago my 
 https://github.com/llvm/llvm-zorg/commit/b6ddf02ce3a54da2df29e7e599b1838167e0e3ad
  was sufficient to fix the issue and was the suggested fix in my opinion.
 Unfortunately nobody on the PowerPC side made the change effective in the 
 build bot. Rather, I received such a heated message 
 (https://reviews.llvm.org/D120305#3347058).
 It was another way to fix the redness (revert) but IMO not justified.
>>>
>>> I feel like we are talking past each other at this point, but in general 
>>> changing the buildbot configuration is not an acceptable solution to a 
>>> broken bot.  Did the bot owner approve that change?
>>
>> Unsure why it isn't acceptable. There is fairly strong evidence that that 
>> specific ppc64le buildbot has a stability issue and 
>> `-DCLANG_DEFAULT_PIE_ON_LINUX=OFF` keeps it as if before this CMake patch.
>> We can always discuss this with @nemanjai in another place, e.g. in IRC if 
>> we want to stop bothering others subscribing to this thread.
>
> It's not acceptable, because buildbots are meant to represent a specific 
> configuration that the buildbot owner cares about.  Changing the buildbot 
> configuration makes the buildbot no longer useful to them since it is now 
> testing something different.  For example, if someone is running production 
> builds that used the same configuration as the buildbot, those production 
> builds are still broken even though the buildbot is green.  Configuration 
> changes like this need to have approval of the bot owner.

If clang-ppc64le-rhel works with `-DCLANG_DEFAULT_PIE_ON_LINUX=off` but not 
with  `-DCLANG_DEFAULT_PIE_ON_LINUX=on`, adding  
`-DCLANG_DEFAULT_PIE_ON_LINUX=off` makes the intention explicit. I am not sure 
why this isn't acceptable.
It's a tech debt, though, as we are making configurations fragmented.

> I still don't understand why you can't revert the patch.  I've encountered 
> this same situation numerous times while working on this project and no one 
> has ever objected this much to doing a revert.  The fact that this is the 
> second time this has happened is concerning to me.

I have stated my reasoning. Configuration churn can also be a problem to users.
Consider what if the DWARF v5 patch got reverted and relanded back and forth. 
Downstream users would keep observing changing behaviors.

In this case, really even normal ppc64le machines (including some bots) were 
happy and just that one was picky.
Given that the bot does not have a high success rate (track record) for at 
least the past month, I am unsure I am supposed to revert my change.

It is more concerning to me that a bot maintainer leaves an unstable bot for so 
long and is not even willing to spend very little time to make a llvm-zorg 
change live (perhaps just restart the bot software), but rather is more willing 
to **write such a long reply taking it very personally**.
(Sorry, I know when I write this, I was a bit in a mood. I felt quite 
frustrated at this point, so I could not keep using the tune when I made the 
reply https://reviews.llvm.org/D120305#3347094)

I can response to you that I have shared the thread to some folks and at least 
two agree with me that nemanjai's reply made the discussion less technical but 
more personal.
And one pointed out that your "nemanjai is correct" message was made a bit 
arbitrary, without evidence of considering the full context.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Tom Stellard via Phabricator via cfe-commits]

tstellar added a comment.

In D120305#3347143 , @MaskRay wrote:

> In D120305#3347142 , @tstellar 
> wrote:
>
>> In D120305#3347139 , @MaskRay 
>> wrote:
>>
>>> In D120305#3347109 , @tstellar 
>>> wrote:
>>>
 [...]
 The issue here has nothing to do with the technical merits of the patch or 
 what the root cause of the problem is.  The policy for this project is 
 that if you commit a patch that breaks someone's configuration (especially 
 a buildbot), then it needs to be fixed quickly or reverted.  I get that 
 this policy can be frustrating as a committer when you feel your patch is 
 correct, and the real problem is elsewhere, but this is still the policy 
 and it should be followed.
>>>
>>> 7 hours ago my 
>>> https://github.com/llvm/llvm-zorg/commit/b6ddf02ce3a54da2df29e7e599b1838167e0e3ad
>>>  was sufficient to fix the issue and was the suggested fix in my opinion.
>>> Unfortunately nobody on the PowerPC side made the change effective in the 
>>> build bot. Rather, I received such a heated message 
>>> (https://reviews.llvm.org/D120305#3347058).
>>> It was another way to fix the redness (revert) but IMO not justified.
>>
>> I feel like we are talking past each other at this point, but in general 
>> changing the buildbot configuration is not an acceptable solution to a 
>> broken bot.  Did the bot owner approve that change?
>
> Unsure why it isn't acceptable. There is fairly strong evidence that that 
> specific ppc64le buildbot has a stability issue and 
> `-DCLANG_DEFAULT_PIE_ON_LINUX=OFF` keeps it as if before this CMake patch.
> We can always discuss this with @nemanjai in another place, e.g. in IRC if we 
> want to stop bothering others subscribing to this thread.

It's not acceptable, because buildbots are meant to represent a specific 
configuration that the buildbot owner cares about.  Changing the buildbot 
configuration makes the buildbot no longer useful to them since it is now 
testing something different.  For example, if someone is running production 
builds that used the same configuration as the buildbot, those production 
builds are still broken even though the buildbot is green.  Configuration 
changes like this need to have approval of the bot owner.

I still don't understand why you can't revert the patch.  I've encountered this 
same situation numerous times while working on this project and no one has ever 
objected this much to doing a revert.  The fact that this is the second time 
this has happened is concerning to me.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D120305#3347142 , @tstellar wrote:

> In D120305#3347139 , @MaskRay wrote:
>
>> In D120305#3347109 , @tstellar 
>> wrote:
>>
>>> [...]
>>> The issue here has nothing to do with the technical merits of the patch or 
>>> what the root cause of the problem is.  The policy for this project is that 
>>> if you commit a patch that breaks someone's configuration (especially a 
>>> buildbot), then it needs to be fixed quickly or reverted.  I get that this 
>>> policy can be frustrating as a committer when you feel your patch is 
>>> correct, and the real problem is elsewhere, but this is still the policy 
>>> and it should be followed.
>>
>> 7 hours ago my 
>> https://github.com/llvm/llvm-zorg/commit/b6ddf02ce3a54da2df29e7e599b1838167e0e3ad
>>  was sufficient to fix the issue and was the suggested fix in my opinion.
>> Unfortunately nobody on the PowerPC side made the change effective in the 
>> build bot. Rather, I received such a heated message 
>> (https://reviews.llvm.org/D120305#3347058).
>> It was another way to fix the redness (revert) but IMO not justified.
>
> I feel like we are talking past each other at this point, but in general 
> changing the buildbot configuration is not an acceptable solution to a broken 
> bot.  Did the bot owner approve that change?

Unsure why it isn't acceptable. There is fairly strong evidence that that 
specific ppc64le buildbot has a stability issue and 
`-DCLANG_DEFAULT_PIE_ON_LINUX=OFF` keeps it as if before this CMake patch.
We can always discuss this with @nemanjai in another place, e.g. in IRC if we 
want to stop bothering others subscribing to this thread.

(The trunk is clean and churn-less (there is no revert so downstream users will 
not get changed behaviors back and forth)).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Tom Stellard via Phabricator via cfe-commits]

tstellar added a comment.

In D120305#3347139 , @MaskRay wrote:

> In D120305#3347109 , @tstellar 
> wrote:
>
>> [...]
>> The issue here has nothing to do with the technical merits of the patch or 
>> what the root cause of the problem is.  The policy for this project is that 
>> if you commit a patch that breaks someone's configuration (especially a 
>> buildbot), then it needs to be fixed quickly or reverted.  I get that this 
>> policy can be frustrating as a committer when you feel your patch is 
>> correct, and the real problem is elsewhere, but this is still the policy and 
>> it should be followed.
>
> 7 hours ago my 
> https://github.com/llvm/llvm-zorg/commit/b6ddf02ce3a54da2df29e7e599b1838167e0e3ad
>  was sufficient to fix the issue and was the suggested fix in my opinion.
> Unfortunately nobody on the PowerPC side made the change effective in the 
> build bot. Rather, I received such a heated message 
> (https://reviews.llvm.org/D120305#3347058).
> It was another way to fix the redness (revert) but IMO not justified.

I feel like we are talking past each other at this point, but in general 
changing the buildbot configuration is not an acceptable solution to a broken 
bot.  Did the bot owner approve that change?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D120305#3347109 , @tstellar wrote:

> [...]
> The issue here has nothing to do with the technical merits of the patch or 
> what the root cause of the problem is.  The policy for this project is that 
> if you commit a patch that breaks someone's configuration (especially a 
> buildbot), then it needs to be fixed quickly or reverted.  I get that this 
> policy can be frustrating as a committer when you feel your patch is correct, 
> and the real problem is elsewhere, but this is still the policy and it should 
> be followed.

7 hours ago my 
https://github.com/llvm/llvm-zorg/commit/b6ddf02ce3a54da2df29e7e599b1838167e0e3ad
 was sufficient to fix the issue and was the suggested fix in my opinion.
Unfortunately nobody on the PowerPC side made the change effective in the build 
bot. Rather, I received such a heated message 
(https://reviews.llvm.org/D120305#3347058).
It was another to fix the redness (revert) but IMO not justified.

And I think I need to push back such request as otherwise the red bot is going 
to waste other contributors' time (see my previous links on "Success Rate").
OK, that took me our hour to reply as a non-native speaker, forgetting that I 
probably should try harder to fix the issue.

Anyway, I find that my previous disabling tests attempt was actually effective. 
274ec425dcc3e3f637dd006c5e9ae33bd0e2e917 
 should 
have appeased https://lab.llvm.org/buildbot/#/builders/57


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

>> error: Segmentation fault (core dumped)

This happens quite often on these ppc bots - they are known for unstability and 
slow cycles.

>> Open https://lab.llvm.org/buildbot/#/builders/57 , click "Success Rate" tab, 
>> the rate is lower compared to other bots recently, even lower than some 
>> other ppc64 bots.

We should also open discussion if such low success rate is “acceptable” for 
LLVM project. Buildbot maintainers are also responsible to provide as stable 
and working bot as possible.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Tom Stellard via Phabricator via cfe-commits]

tstellar added a comment.

In D120305#3347108 , @MaskRay wrote:

> In D120305#3347103 , @tstellar 
> wrote:
>
>> In D120305#3347058 , @nemanjai 
>> wrote:
>>
>>> In D120305#3346880 , @MaskRay 
>>> wrote:
>>>
 While I feel sorry for leaving clang-ppc64le-rhel red for some time and am 
 willing to fix issues if I have access to a ppc64 machine (especially 
 compiler-rt ones that I care about),
 I feel uncomfortably if a group just bluntly request "please pull this 
 patch" when apparently (a) there is a better approach (explicitly setting 
 CLANG_DEFAULT_PIE_ON_LINUX=OFF) and (b) there is something a bot 
 maintainer can do
 and (c) there is just some inherent stability problem (in this case, 
 consider not enabling the testing when the target is still unstable) that 
 is causing not only this issue, but various other reports (as I watch 
 sanitizer failures quite closely and ppc64 often tends to be the outlier 
 thing)
>>>
>>> Statements like this seem to be at odds with this community's culture (or 
>>> at least the way I understand it).
>>> As long as I have been a member of this community, the guidance for patches 
>>> that break bots is to fix it immediately if the fix is obvious/trivial and 
>>> if it isn't, to pull the patch until a solution can be found. I am not 
>>> aware of any changes to this policy. I would also like to add that this 
>>> approach serves most other members of the community quite well and at least 
>>> I personally don't see much opposition to this. Frankly, the only person I 
>>> have ever received a response that amounts to "I would rather not" when 
>>> asking them to pull a patch that breaks bots is yourself.
>>
>> @nemanjai Is correct here.
>
> May I beg that you read my reply first and give more evidence when standing 
> by one party?
> You as a https://foundation.llvm.org/docs/board/ member, your words weigh a 
> lot, but with great power comes great responsibility, so every judgement 
> needs to be made prudently.
>
>> @MaskRay  I feel like we are starting to repeat the same discussion we had 
>> with the start-stop-gc patches, and I would like to have a better outcome 
>> this time.  Can you please just revert the patch and then we can discuss the 
>> next steps.
>
> Everyone wants to discuss start-stop-gc can go to 
> https://discourse.llvm.org/t/lld-default-nostart-stop-gc-default-on-release-13-x-and-main/5369
> Please don't conflate things here. I felt bad that people used an 
> unguaranteed behavior as granted and accused/attacked me of changes.
> I think time has proved my correctness: I don't think anyone sees new wacky 
> things in this area.

The issue here has nothing to do with the technical merits of the patch or what 
the root cause of the problem is.  The policy for this project is that if you 
commit a patch that breaks someone's configuration (especially a buildbot), 
then it needs to be fixed quickly or reverted.  I get that this policy can be 
frustrating as a committer when you feel your patch is correct, and the real 
problem is elsewhere, but this is still the policy and it should be followed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D120305#3347103 , @tstellar wrote:

> In D120305#3347058 , @nemanjai 
> wrote:
>
>> In D120305#3346880 , @MaskRay 
>> wrote:
>>
>>> While I feel sorry for leaving clang-ppc64le-rhel red for some time and am 
>>> willing to fix issues if I have access to a ppc64 machine (especially 
>>> compiler-rt ones that I care about),
>>> I feel uncomfortably if a group just bluntly request "please pull this 
>>> patch" when apparently (a) there is a better approach (explicitly setting 
>>> CLANG_DEFAULT_PIE_ON_LINUX=OFF) and (b) there is something a bot maintainer 
>>> can do
>>> and (c) there is just some inherent stability problem (in this case, 
>>> consider not enabling the testing when the target is still unstable) that 
>>> is causing not only this issue, but various other reports (as I watch 
>>> sanitizer failures quite closely and ppc64 often tends to be the outlier 
>>> thing)
>>
>> Statements like this seem to be at odds with this community's culture (or at 
>> least the way I understand it).
>> As long as I have been a member of this community, the guidance for patches 
>> that break bots is to fix it immediately if the fix is obvious/trivial and 
>> if it isn't, to pull the patch until a solution can be found. I am not aware 
>> of any changes to this policy. I would also like to add that this approach 
>> serves most other members of the community quite well and at least I 
>> personally don't see much opposition to this. Frankly, the only person I 
>> have ever received a response that amounts to "I would rather not" when 
>> asking them to pull a patch that breaks bots is yourself.
>
> @nemanjai Is correct here.

May I beg that you read my reply first and give more evidence when standing by 
one party?
You as a https://foundation.llvm.org/docs/board/ member, your words weigh a 
lot, but with great power comes great responsibility, so every judgement needs 
to be made prudently.

> @MaskRay  I feel like we are starting to repeat the same discussion we had 
> with the start-stop-gc patches, and I would like to have a better outcome 
> this time.  Can you please just revert the patch and then we can discuss the 
> next steps.

Everyone wants to discuss start-stop-gc can go to 
https://discourse.llvm.org/t/lld-default-nostart-stop-gc-default-on-release-13-x-and-main/5369
Please don't conflate things here. I felt bad that people used an unguaranteed 
behavior as granted and accused/attacked me of changes.
I think time has proved my correctness: I don't think anyone sees new wacky 
things in this area.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Tom Stellard via Phabricator via cfe-commits]

tstellar added a comment.

In D120305#3347058 , @nemanjai wrote:

> In D120305#3346880 , @MaskRay wrote:
>
>> While I feel sorry for leaving clang-ppc64le-rhel red for some time and am 
>> willing to fix issues if I have access to a ppc64 machine (especially 
>> compiler-rt ones that I care about),
>> I feel uncomfortably if a group just bluntly request "please pull this 
>> patch" when apparently (a) there is a better approach (explicitly setting 
>> CLANG_DEFAULT_PIE_ON_LINUX=OFF) and (b) there is something a bot maintainer 
>> can do
>> and (c) there is just some inherent stability problem (in this case, 
>> consider not enabling the testing when the target is still unstable) that is 
>> causing not only this issue, but various other reports (as I watch sanitizer 
>> failures quite closely and ppc64 often tends to be the outlier thing)
>
> Statements like this seem to be at odds with this community's culture (or at 
> least the way I understand it).
> As long as I have been a member of this community, the guidance for patches 
> that break bots is to fix it immediately if the fix is obvious/trivial and if 
> it isn't, to pull the patch until a solution can be found. I am not aware of 
> any changes to this policy. I would also like to add that this approach 
> serves most other members of the community quite well and at least I 
> personally don't see much opposition to this. Frankly, the only person I have 
> ever received a response that amounts to "I would rather not" when asking 
> them to pull a patch that breaks bots is yourself.

@nemanjai Is correct here.

@MaskRay  I feel like we are starting to repeat the same discussion we had with 
the start-stop-gc patches, and I would like to have a better outcome this time. 
 Can you please just revert the patch and then we can discuss the next steps.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D120305#3347058 , @nemanjai wrote:

> 



> Statements like this seem to be at odds with this community's culture (or at 
> least the way I understand it).
> As long as I have been a member of this community, the guidance for patches 
> that break bots is to fix it immediately if the fix is obvious/trivial and if 
> it isn't, to pull the patch until a solution can be found. I am not aware of 
> any changes to this policy. I would also like to add that this approach 
> serves most other members of the community quite well and at least I 
> personally don't see much opposition to this. Frankly, the only person I have 
> ever received a response that amounts to "I would rather not" when asking 
> them to pull a patch that breaks bots is yourself.

Hi @nemanjai, when I mentioned "as I watch sanitizer failures quite closely and 
ppc64 often tends to be the outlier thing", I was talking about the build bots.
However, you seem to take it very personally, conflate it with a judgement to 
the PowerPC target, and try to defend for the PowerPC target.
I think perhaps you become too emotional from this reply. Hope we can clam down.

> So I'll try to respond to the individual statements you have made here.
>
> No access to a PowerPC machine - we have given you access to one before and 
> are happy to do so again at any time.

Ack. Thanks for that.

> "bluntly requesting to pull the patch" - This is perhaps the part of your 
> statement that I find most surprising. In case you haven't come across this, 
> I encourage you to have a read through it. If you feel this needs to change, 
> I encourage you to bring it up for discussion with the wider community. Of 
> particular interest is this sentence: We strongly encourage “revert to green” 
> as opposed to “fixing forward”. We encourage reverting first, investigating 
> offline, and then reapplying the fixed patch - possibly after another round 
> of review if warranted.

Sure, I know this policy. See below.

> "there is a better approach" - I don't think I have to spend a lot of time 
> explaining how subjective this statement is.
> "there is something that a bot maintainer can do" - I can't quite decipher 
> whether this is a disingenuous statement pretending that this is a situation 
> where the bot maintainer (effectively myself in this case) isn't willing to 
> help. Or perhaps it is a statement you made in the heat of the moment when I 
> asked you to pull the patch and you missed the part where I offered help to 
> resolve the issues when normal business resumes. I will give you the benefit 
> of assuming the latter as I truly don't believe you have ill intentions here. 
> I would just like to add that, as you surely realize, bot maintainers are not 
> sitting around waiting for someone to break their bot so they can jump in 
> immediately and work on resolving the issue. As contributors to this project, 
> it is our responsibility to keep the tip of trunk green and to work with bot 
> maintainers on their own time (within reason) to resolve issues we cause on 
> their bots.

The current issue is that one ppc64 bot (clang-ppc64le-rhel) was complaing 
about some sanitizer tests while other ppc64 bots testing sanitizers (e.g. 
clang-ppc64le-linux-multistage sanitizer-ppc64be-linux sanitizer-ppc64le-linux) 
were apparently happy.
This is an important factor making me believe this is a build bot stability 
issue, not the patch's fault.

From the failure log, the diagnostic looks like:

  :1:11: note: possible intended match here
  error: Segmentation fault (core dumped)

This is weird. This was not the first time I saw this "Segmentation fault".
In the past, I saw GNU ld crashes or similar weird segfaults (on perhaps this 
bot).

-fPIE -pie is a very common configuration tested extensively by users and a 
bunch of ppc64 distributions.
In the downstream distributors typically patch Clang to behave the similar way 
as GCC, as this CMake patch tried to do to reduce downstream 
patch/customization burden.

Making use of my expertise and experience, I put up this statement fairly 
responsibly: this really looks like the problem with this bot, or sanitizer 
tests got enabled while some inherent stability issues havn't been addressed.

Perhaps sanitizers are not so compatible with the bot's environment (say glibc).
If so, the issue will pop up from time to time and a bot maintainer may jump 
out to every patch which tickles the problem.

This is just going to waste developer time.
There is a very high probability that a patch author may shortgun and revert 
the wrong patch, causing unneeded churn.

I understand that a bot maintainer does not bandwidth to watch every change.
But this is the very time that we would appreciate that a bot maintainer did a 
little work just to make everyone happy by drop testing the flaky tests.
I realized that I would not suggest adding UNSUPPORTED to test files since that 

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

In D120305#3346880 , @MaskRay wrote:

> While I feel sorry for leaving clang-ppc64le-rhel red for some time and am 
> willing to fix issues if I have access to a ppc64 machine (especially 
> compiler-rt ones that I care about),
> I feel uncomfortably if a group just bluntly request "please pull this patch" 
> when apparently (a) there is a better approach (explicitly setting 
> CLANG_DEFAULT_PIE_ON_LINUX=OFF) and (b) there is something a bot maintainer 
> can do
> and (c) there is just some inherent stability problem (in this case, consider 
> not enabling the testing when the target is still unstable) that is causing 
> not only this issue, but various other reports (as I watch sanitizer failures 
> quite closely and ppc64 often tends to be the outlier thing)

Statements like this seem to be at odds with this community's culture (or at 
least the way I understand it).
As long as I have been a member of this community, the guidance for patches 
that break bots is to fix it immediately if the fix is obvious/trivial and if 
it isn't, to pull the patch until a solution can be found. I am not aware of 
any changes to this policy. I would also like to add that this approach serves 
most other members of the community quite well and at least I personally don't 
see much opposition to this. Frankly, the only person I have ever received a 
response that amounts to "I would rather not" when asking them to pull a patch 
that breaks bots is yourself.

So I'll try to respond to the individual statements you have made here.

1. No access to a PowerPC machine - we have given you access to one before and 
are happy to do so again at any time.
2. "bluntly requesting to pull the patch" - This is perhaps the part of your 
statement that I find most surprising. In case you haven't come across this 
, I 
encourage you to have a read through it. If you feel this needs to change, I 
encourage you to bring it up for discussion with the wider community. Of 
particular interest is this sentence: `We strongly encourage “revert to green” 
as opposed to “fixing forward”. We encourage reverting first, investigating 
offline, and then reapplying the fixed patch - possibly after another round of 
review if warranted.`
3. "there is a better approach" - I don't think I have to spend a lot of time 
explaining how subjective this statement is.
4. "there is something that a bot maintainer can do" - I can't quite decipher 
whether this is a disingenuous statement pretending that this is a situation 
where the bot maintainer (effectively myself in this case) isn't willing to 
help. Or perhaps it is a statement you made in the heat of the moment when I 
asked you to pull the patch and you missed the part where I offered help to 
resolve the issues when normal business resumes. I will give you the benefit of 
assuming the latter as I truly don't believe you have ill intentions here. I 
would just like to add that, as you surely realize, bot maintainers are not 
sitting around waiting for someone to break their bot so they can jump in 
immediately and work on resolving the issue. As contributors to this project, 
it is our responsibility to keep the tip of trunk green and to work with bot 
maintainers on their own time (within reason) to resolve issues we cause on 
their bots.
5. The rather surprising and discouraging statement you made under `(c)` - 
while I realize that PowerPC may not be a target on which you develop, it is 
really not fair to make a blanket statement that PowerPC is not stable wrt. 
sanitizers. If you feel there is a stability issue with a specific bot or a 
specific target, I encourage you to collect data about false failures and bring 
it up with us - we would be happy to look into it as I am sure would any other 
target/bot maintainer. Statements like this sow the seeds of resentment towards 
specific targets - you are effectively saying that PowerPC is an unstable 
target (at least wrt. to sanitizers) but we insist on burdening the community 
with our unstable target by running sanitizer tests in our bots. I am afraid 
that unlike number 4. above, I don't find any ambiguity in your statement here. 
Your statement seems to be clearly and unambiguously hostile towards PowerPC 
and as such, I find it at odds with the spirit of this community. Regardless of 
all of that, I would once again like to reiterate that I would be very happy to 
look into false failures you are encountering with sanitizers on PowerPC.

Fangrui, I would really like you to understand that I very much value your 
contribution to various LLVM projects. You are an exceptional developer and 
seem to be laser focused on advancing the state of LLVM and its subprojects. 
This is not at all lost on me. However, you need to understand that this 
community has all kinds of participants, many of which also care about older or 

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D120305#3346947 , @tstellar wrote:

> In D120305#3346880 , @MaskRay wrote:
>
>> In D120305#3346787 , @nemanjai 
>> wrote:
>>
>>> In D120305#3346144 , @MaskRay 
>>> wrote:
>>>
 In D120305#3345978 , @MaskRay 
 wrote:

> In D120305#3345810 , @RKSimon 
> wrote:
>
>> @MaskRay The ppc buildbots have been red since these patches - please 
>> can you take a look? 
>> https://lab.llvm.org/buildbot/#/builders/57/builds/15454
>
> Seems that ppc64 doesn't support test/sanitizer_common test/crt -fpie. 
> I'll just disable them: https://github.com/llvm/llvm-project/issues/54084

 Actually I don't know how to disable the tests: 
 https://lab.llvm.org/buildbot/#/builders/57/builds/15497 still failed.
 Hope someone from #powerpc  can 
 disable them.
>>>
>>> This does not appear to be a matter of simply marking some tests as 
>>> UNSUPPORTED. Since this landed, there have been many builds with different 
>>> sanitizer failures and different numbers of sanitizer failures. Please pull 
>>> this patch to bring the bots back to green and we can work with you next 
>>> week on fixing what needs to be fixed.
>>
>> I enabled -DCLANG_DEFAULT_PIE_ON_LINUX=OFF for clang-ppc64le-rhel: 
>> https://github.com/llvm/llvm-zorg/commit/b6ddf02ce3a54da2df29e7e599b1838167e0e3ad
>> which should fix the issues.
>>
>> While I feel sorry for leaving clang-ppc64le-rhel red for some time and am 
>> willing to fix issues if I have access to a ppc64 machine (especially 
>> compiler-rt ones that I care about),
>> I feel uncomfortably if a group just bluntly request "please pull this 
>> patch" when apparently (a) there is a better approach (explicitly setting 
>> CLANG_DEFAULT_PIE_ON_LINUX=OFF) and (b) there is something a bot maintainer 
>> can do
>> and (c) there is just some inherent stability problem (in this case, 
>> consider not enabling the testing when the target is still unstable) that is 
>> causing not only this issue, but various other reports (as I watch sanitizer 
>> failures quite closely and ppc64 often tends to be the outlier thing)
>
> Fixing the buildbot like this doesn't help regular users though.  I think 
> it's better to revert and then work on a solution as @nemanjai suggested.  
> What  are the downsides to reverting this?

Well, I can speak for regular ppc64le users now since q66 kindly gives me 
access to a ppc64le machine.
`check-lsan check-sanitizer check-crt` etc passed fairly well with default PIE 
Clang.

I think we previously paid too less attention on build bot stability problems.
In this case I suspected again this is such a problem (typically sanitizer not 
so compatible with some older glibc/binutils)

  :1:16: note: possible intended match here
  error: Segmentation fault (core dumped)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Tom Stellard via Phabricator via cfe-commits]

tstellar added a comment.

In D120305#3346880 , @MaskRay wrote:

> In D120305#3346787 , @nemanjai 
> wrote:
>
>> In D120305#3346144 , @MaskRay 
>> wrote:
>>
>>> In D120305#3345978 , @MaskRay 
>>> wrote:
>>>
 In D120305#3345810 , @RKSimon 
 wrote:

> @MaskRay The ppc buildbots have been red since these patches - please can 
> you take a look? https://lab.llvm.org/buildbot/#/builders/57/builds/15454

 Seems that ppc64 doesn't support test/sanitizer_common test/crt -fpie. 
 I'll just disable them: https://github.com/llvm/llvm-project/issues/54084
>>>
>>> Actually I don't know how to disable the tests: 
>>> https://lab.llvm.org/buildbot/#/builders/57/builds/15497 still failed.
>>> Hope someone from #powerpc  can 
>>> disable them.
>>
>> This does not appear to be a matter of simply marking some tests as 
>> UNSUPPORTED. Since this landed, there have been many builds with different 
>> sanitizer failures and different numbers of sanitizer failures. Please pull 
>> this patch to bring the bots back to green and we can work with you next 
>> week on fixing what needs to be fixed.
>
> I enabled -DCLANG_DEFAULT_PIE_ON_LINUX=OFF for clang-ppc64le-rhel: 
> https://github.com/llvm/llvm-zorg/commit/b6ddf02ce3a54da2df29e7e599b1838167e0e3ad
> which should fix the issues.
>
> While I feel sorry for leaving clang-ppc64le-rhel red for some time and am 
> willing to fix issues if I have access to a ppc64 machine (especially 
> compiler-rt ones that I care about),
> I feel uncomfortably if a group just bluntly request "please pull this patch" 
> when apparently (a) there is a better approach (explicitly setting 
> CLANG_DEFAULT_PIE_ON_LINUX=OFF) and (b) there is something a bot maintainer 
> can do
> and (c) there is just some inherent stability problem (in this case, consider 
> not enabling the testing when the target is still unstable) that is causing 
> not only this issue, but various other reports (as I watch sanitizer failures 
> quite closely and ppc64 often tends to be the outlier thing)

Fixing the buildbot like this doesn't help regular users though.  I think it's 
better to revert and then work on a solution as @nemanjai suggested.  What  are 
the downsides to reverting this?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D120305#3346787 , @nemanjai wrote:

> In D120305#3346144 , @MaskRay wrote:
>
>> In D120305#3345978 , @MaskRay 
>> wrote:
>>
>>> In D120305#3345810 , @RKSimon 
>>> wrote:
>>>
 @MaskRay The ppc buildbots have been red since these patches - please can 
 you take a look? https://lab.llvm.org/buildbot/#/builders/57/builds/15454
>>>
>>> Seems that ppc64 doesn't support test/sanitizer_common test/crt -fpie. I'll 
>>> just disable them: https://github.com/llvm/llvm-project/issues/54084
>>
>> Actually I don't know how to disable the tests: 
>> https://lab.llvm.org/buildbot/#/builders/57/builds/15497 still failed.
>> Hope someone from #powerpc  can 
>> disable them.
>
> This does not appear to be a matter of simply marking some tests as 
> UNSUPPORTED. Since this landed, there have been many builds with different 
> sanitizer failures and different numbers of sanitizer failures. Please pull 
> this patch to bring the bots back to green and we can work with you next week 
> on fixing what needs to be fixed.

I enabled -DCLANG_DEFAULT_PIE_ON_LINUX=OFF for clang-ppc64le-rhel: 
https://github.com/llvm/llvm-zorg/commit/b6ddf02ce3a54da2df29e7e599b1838167e0e3ad
which should fix the issues.

While I feel sorry for leaving clang-ppc64le-rhel red for some time and am 
willing to fix issues if I have access to a ppc64 machine (especially 
compiler-rt ones that I care about),
I feel uncomfortably if a group just bluntly request "please pull this patch" 
when apparently (a) there is a better approach (explicitly setting 
CLANG_DEFAULT_PIE_ON_LINUX=OFF) and (b) there is something a bot maintainer can 
do
and (c) there is just some inherent stability problem (in this case, consider 
not enabling the testing when the target is still unstable).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

In D120305#3346144 , @MaskRay wrote:

> In D120305#3345978 , @MaskRay wrote:
>
>> In D120305#3345810 , @RKSimon 
>> wrote:
>>
>>> @MaskRay The ppc buildbots have been red since these patches - please can 
>>> you take a look? https://lab.llvm.org/buildbot/#/builders/57/builds/15454
>>
>> Seems that ppc64 doesn't support test/sanitizer_common test/crt -fpie. I'll 
>> just disable them: https://github.com/llvm/llvm-project/issues/54084
>
> Actually I don't know how to disable the tests: 
> https://lab.llvm.org/buildbot/#/builders/57/builds/15497 still failed.
> Hope someone from #powerpc  can 
> disable them.

This does not appear to be a matter of simply marking some tests as 
UNSUPPORTED. Since this landed, there have been many builds with different 
sanitizer failures and different numbers of sanitizer failures. Please pull 
this patch to bring the bots back to green and we can work with you next week 
on fixing what needs to be fixed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D120305#3345978 , @MaskRay wrote:

> In D120305#3345810 , @RKSimon wrote:
>
>> @MaskRay The ppc buildbots have been red since these patches - please can 
>> you take a look? https://lab.llvm.org/buildbot/#/builders/57/builds/15454
>
> Seems that ppc64 doesn't support test/sanitizer_common test/crt -fpie. I'll 
> just disable them: https://github.com/llvm/llvm-project/issues/54084

Actually I don't know how to disable the tests: 
https://lab.llvm.org/buildbot/#/builders/57/builds/15497 still failed.
Hope someone from #powerpc  can disable 
them.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

In D120305#3344938 , @nikic wrote:

> Hm, it looks like enabling PIE has a pretty big negative compile-time impact, 
> with a 20% regression on sqlite3: 
> http://llvm-compile-time-tracker.com/compare.php?from=611122892e6d5813444bdd0e1fbe0a96f6e09779=3c4ed02698afec021c6bca80740d1e58e3ee019e=instructions
>  Code-size on kimwitu++ regresses by 13%.
>
> Is that kind of impact expected?

almost no impact on legacy pm, interesting..


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D120305#3345810 , @RKSimon wrote:

> @MaskRay The ppc buildbots have been red since these patches - please can you 
> take a look? https://lab.llvm.org/buildbot/#/builders/57/builds/15454

Seems that ppc64 doesn't support lsan -fpie. I'll just disable it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Simon Pilgrim via Phabricator via cfe-commits]

RKSimon added a comment.

@MaskRay The ppc buildbots have been red since these patches - please can you 
take a look? https://lab.llvm.org/buildbot/#/builders/57/builds/15454


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D120305#3344938 , @nikic wrote:

> Hm, it looks like enabling PIE has a pretty big negative compile-time impact, 
> with a 20% regression on sqlite3: 
> http://llvm-compile-time-tracker.com/compare.php?from=611122892e6d5813444bdd0e1fbe0a96f6e09779=3c4ed02698afec021c6bca80740d1e58e3ee019e=instructions
>  Code-size on kimwitu++ regresses by 13%.
>
> Is that kind of impact expected?

It's not expected on aarch64/x86-64. Some architectures not supporting 
PC-relative relocations (i386/ppc32) may suffer and that's expected.

We need to investigate the -fno-pic vs -fpie difference.
Many groups already use PIE and the investigation will be useful.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

Hm, it looks like enabling PIE has a pretty big negative compile-time impact, 
with a 20% regression on sqlite3: 
http://llvm-compile-time-tracker.com/compare.php?from=611122892e6d5813444bdd0e1fbe0a96f6e09779=3c4ed02698afec021c6bca80740d1e58e3ee019e=instructions
 Code-size on kimwitu++ regresses by 13%.

Is that kind of impact expected?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Fangrui Song via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG3c4ed02698af: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to 
ON (authored by MaskRay).

Changed prior to commit:
  https://reviews.llvm.org/D120305?vs=411016=411259#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

Files:
  clang/CMakeLists.txt
  clang/docs/ReleaseNotes.rst
  clang/test/Driver/hip-fpie-option.hip
  llvm/utils/gn/secondary/clang/include/clang/Config/BUILD.gn
  utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h


Index: utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h
===
--- utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h
+++ utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h
@@ -23,7 +23,7 @@
 #define BUG_REPORT_URL "https://github.com/llvm/llvm-project/issues/;
 
 /* Default to -fPIE and -pie on Linux. */
-#define CLANG_DEFAULT_PIE_ON_LINUX 0
+#define CLANG_DEFAULT_PIE_ON_LINUX 1
 
 /* Default linker to use. */
 #define CLANG_DEFAULT_LINKER ""
Index: llvm/utils/gn/secondary/clang/include/clang/Config/BUILD.gn
===
--- llvm/utils/gn/secondary/clang/include/clang/Config/BUILD.gn
+++ llvm/utils/gn/secondary/clang/include/clang/Config/BUILD.gn
@@ -9,7 +9,7 @@
   output = "$target_gen_dir/config.h"
   values = [
 "BUG_REPORT_URL=https://github.com/llvm/llvm-project/issues/;,
-"CLANG_DEFAULT_PIE_ON_LINUX=",
+"CLANG_DEFAULT_PIE_ON_LINUX=1",
 "CLANG_DEFAULT_LINKER=",
 "CLANG_DEFAULT_STD_C=",
 "CLANG_DEFAULT_STD_CXX=",
Index: clang/test/Driver/hip-fpie-option.hip
===
--- clang/test/Driver/hip-fpie-option.hip
+++ clang/test/Driver/hip-fpie-option.hip
@@ -1,15 +1,15 @@
-// REQUIRES: clang-driver, amdgpu-registered-target
+// REQUIRES: clang-driver, amdgpu-registered-target, default-pie-on-linux
 
 // -fPIC and -fPIE only affects host relocation model.
 // device compilation always uses PIC. 
 
 // RUN: %clang -### -target x86_64-unknown-linux-gnu \
 // RUN:   --offload-arch=gfx906 %s -nogpulib -nogpuinc \
-// RUN:   2>&1 | FileCheck -check-prefixes=DEV,HOST-STATIC %s
+// RUN:   2>&1 | FileCheck -check-prefixes=DEV,HOST-PIE %s
 
 // RUN: %clang -### -target x86_64-unknown-linux-gnu \
 // RUN:   -fgpu-rdc --offload-arch=gfx906 %s -nogpulib -nogpuinc \
-// RUN:   2>&1 | FileCheck -check-prefixes=DEV,HOST-STATIC %s
+// RUN:   2>&1 | FileCheck -check-prefixes=DEV,HOST-PIE %s
 
 // RUN: %clang -### -target x86_64-unknown-linux-gnu \
 // RUN:   --offload-arch=gfx906 %s -nogpulib -nogpuinc \
@@ -32,7 +32,6 @@
 // RUN:   2>&1 | FileCheck -check-prefixes=DEV,HOST-PIE %s
 
 // DEV-DAG: {{".*clang.*".* "-triple" "amdgcn-amd-amdhsa".* 
"-mrelocation-model" "pic" "-pic-level" "[1|2]".* "-mframe-pointer=all"}}
-// HOST-STATIC-DAG: {{".*clang.*".* "-triple" "x86_64-unknown-linux-gnu".* 
"-mrelocation-model" "static"}}
 // HOST-PIC-DAG: {{".*clang.*".* "-triple" "x86_64-unknown-linux-gnu".* 
"-mrelocation-model" "pic" "-pic-level" "2"}}
 // HOST-PIC-NOT: "-pic-is-pie"
 // HOST-PIE-DAG: {{".*clang.*".* "-triple" "x86_64-unknown-linux-gnu".* 
"-mrelocation-model" "pic" "-pic-level" "2" "-pic-is-pie"}}
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -183,6 +183,12 @@
 Build System Changes
 
 
+* CMake ``-DCLANG_DEFAULT_PIE_ON_LINUX=ON`` is now the default. This is used by
+  linux-gnu systems to decide whether ``-fPIE -pie`` is the default (instead of
+  ``-fno-pic -no-pie``). This matches GCC installations on many Linux distros.
+  Note: linux-android and linux-musl always default to ``-fPIE -pie``, ignoring
+  this variable. ``-DCLANG_DEFAULT_PIE_ON_LINUX`` will be removed in the 
future.
+
 AST Matchers
 
 
Index: clang/CMakeLists.txt
===
--- clang/CMakeLists.txt
+++ clang/CMakeLists.txt
@@ -245,7 +245,7 @@
 set(CLANG_SPAWN_CC1 OFF CACHE BOOL
 "Whether clang should use a new process for the CC1 invocation")
 
-option(CLANG_DEFAULT_PIE_ON_LINUX "Default to -fPIE and -pie on Linux" OFF)
+option(CLANG_DEFAULT_PIE_ON_LINUX "Default to -fPIE and -pie on linux-gnu" ON)
 
 # TODO: verify the values against LangStandards.def?
 set(CLANG_DEFAULT_STD_C "" CACHE STRING


Index: utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h
===
--- utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h
+++ utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h
@@ -23,7 +23,7 @@
 #define 

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

I'll test both CLANG_DEFAULT_PIE_ON_LINUX=on and 
CLANG_DEFAULT_PIE_ON_LINUX=off, and then push.
This is long undue. I did not change the default when 
CLANG_DEFAULT_PIE_ON_LINUX was added to avoid changes to release/14.x.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Sam James via Phabricator via cfe-commits]

thesamesam accepted this revision.
thesamesam added a comment.
This revision is now accepted and ready to land.

Been running with this for some time now (before we added the option). We 
should address the tests but they look sorted now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Fangrui Song via Phabricator via cfe-commits]

MaskRay updated this revision to Diff 411016.
MaskRay marked an inline comment as done.
MaskRay added a comment.

Explain pie


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

Files:
  clang/CMakeLists.txt
  clang/docs/ReleaseNotes.rst
  clang/test/Driver/hexagon-toolchain-elf.c
  clang/test/Driver/hip-fpie-option.hip
  llvm/utils/gn/secondary/clang/include/clang/Config/BUILD.gn
  utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h


Index: utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h
===
--- utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h
+++ utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h
@@ -23,7 +23,7 @@
 #define BUG_REPORT_URL "https://github.com/llvm/llvm-project/issues/;
 
 /* Default to -fPIE and -pie on Linux. */
-#define CLANG_DEFAULT_PIE_ON_LINUX 0
+#define CLANG_DEFAULT_PIE_ON_LINUX 1
 
 /* Default linker to use. */
 #define CLANG_DEFAULT_LINKER ""
Index: llvm/utils/gn/secondary/clang/include/clang/Config/BUILD.gn
===
--- llvm/utils/gn/secondary/clang/include/clang/Config/BUILD.gn
+++ llvm/utils/gn/secondary/clang/include/clang/Config/BUILD.gn
@@ -9,7 +9,7 @@
   output = "$target_gen_dir/config.h"
   values = [
 "BUG_REPORT_URL=https://github.com/llvm/llvm-project/issues/;,
-"CLANG_DEFAULT_PIE_ON_LINUX=",
+"CLANG_DEFAULT_PIE_ON_LINUX=1",
 "CLANG_DEFAULT_LINKER=",
 "CLANG_DEFAULT_STD_C=",
 "CLANG_DEFAULT_STD_CXX=",
Index: clang/test/Driver/hip-fpie-option.hip
===
--- clang/test/Driver/hip-fpie-option.hip
+++ clang/test/Driver/hip-fpie-option.hip
@@ -1,15 +1,15 @@
-// REQUIRES: clang-driver, amdgpu-registered-target
+// REQUIRES: clang-driver, amdgpu-registered-target, default-pie-on-linux
 
 // -fPIC and -fPIE only affects host relocation model.
 // device compilation always uses PIC. 
 
 // RUN: %clang -### -target x86_64-unknown-linux-gnu \
 // RUN:   --offload-arch=gfx906 %s -nogpulib -nogpuinc \
-// RUN:   2>&1 | FileCheck -check-prefixes=DEV,HOST-STATIC %s
+// RUN:   2>&1 | FileCheck -check-prefixes=DEV,HOST-PIE %s
 
 // RUN: %clang -### -target x86_64-unknown-linux-gnu \
 // RUN:   -fgpu-rdc --offload-arch=gfx906 %s -nogpulib -nogpuinc \
-// RUN:   2>&1 | FileCheck -check-prefixes=DEV,HOST-STATIC %s
+// RUN:   2>&1 | FileCheck -check-prefixes=DEV,HOST-PIE %s
 
 // RUN: %clang -### -target x86_64-unknown-linux-gnu \
 // RUN:   --offload-arch=gfx906 %s -nogpulib -nogpuinc \
@@ -32,7 +32,6 @@
 // RUN:   2>&1 | FileCheck -check-prefixes=DEV,HOST-PIE %s
 
 // DEV-DAG: {{".*clang.*".* "-triple" "amdgcn-amd-amdhsa".* 
"-mrelocation-model" "pic" "-pic-level" "[1|2]".* "-mframe-pointer=all"}}
-// HOST-STATIC-DAG: {{".*clang.*".* "-triple" "x86_64-unknown-linux-gnu".* 
"-mrelocation-model" "static"}}
 // HOST-PIC-DAG: {{".*clang.*".* "-triple" "x86_64-unknown-linux-gnu".* 
"-mrelocation-model" "pic" "-pic-level" "2"}}
 // HOST-PIC-NOT: "-pic-is-pie"
 // HOST-PIE-DAG: {{".*clang.*".* "-triple" "x86_64-unknown-linux-gnu".* 
"-mrelocation-model" "pic" "-pic-level" "2" "-pic-is-pie"}}
Index: clang/test/Driver/hexagon-toolchain-elf.c
===
--- clang/test/Driver/hexagon-toolchain-elf.c
+++ clang/test/Driver/hexagon-toolchain-elf.c
@@ -1,3 +1,4 @@
+// UNSUPPORTED: default-pie-on-linux
 // 
-
 // Test standard include paths
 // 
-
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -183,6 +183,10 @@
 Build System Changes
 
 
+* CMake ``-DCLANG_DEFAULT_PIE_ON_LINUX=ON`` is now the default.
+  This matches GCC installations on many Linux distros: default to ``-fPIE 
-pie``.
+  When the variable is OFF, linux-gnu systems default to ``-fno-pic -no-pie``.
+
 AST Matchers
 
 
Index: clang/CMakeLists.txt
===
--- clang/CMakeLists.txt
+++ clang/CMakeLists.txt
@@ -245,7 +245,7 @@
 set(CLANG_SPAWN_CC1 OFF CACHE BOOL
 "Whether clang should use a new process for the CC1 invocation")
 
-option(CLANG_DEFAULT_PIE_ON_LINUX "Default to -fPIE and -pie on Linux" OFF)
+option(CLANG_DEFAULT_PIE_ON_LINUX "Default to -fPIE and -pie on Linux" ON)
 
 # TODO: verify the values against LangStandards.def?
 set(CLANG_DEFAULT_STD_C "" CACHE STRING


Index: utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h
===
--- 

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Simon Atanasyan via Phabricator via cfe-commits]

atanasyan added a comment.

In D120305#3337045 , @atanasyan wrote:

> In D120305#3337019 , @MaskRay wrote:
>
>> @atanasyan A few mips test/Driver tests will fail. Wonder if you have time 
>> making them more portable... Otherwise I'll just add `UNSUPPORTED: 
>> default-pie-on-linux` to them.
>>
>>   Failed Tests (6):
>> Clang :: Driver/hip-fpie-option.hip
>> Clang :: Driver/mips-cs.cpp
>> Clang :: Driver/mips-fsf.cpp
>> Clang :: Driver/mips-img-v2.cpp
>> Clang :: Driver/mips-img.cpp
>> Clang :: Driver/mips-mti-linux.c
>
> I will take a look at the tests. Probably `hip-fpie-option.hip` is unrelated 
> to MIPS?

Fixed MIPS tests at cedc23b 



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Simon Atanasyan via Phabricator via cfe-commits]

atanasyan added a comment.

In D120305#3337019 , @MaskRay wrote:

> @atanasyan A few mips test/Driver tests will fail. Wonder if you have time 
> making them more portable... Otherwise I'll just add `UNSUPPORTED: 
> default-pie-on-linux` to them.
>
>   Failed Tests (6):
> Clang :: Driver/hip-fpie-option.hip
> Clang :: Driver/mips-cs.cpp
> Clang :: Driver/mips-fsf.cpp
> Clang :: Driver/mips-img-v2.cpp
> Clang :: Driver/mips-img.cpp
> Clang :: Driver/mips-mti-linux.c

I will take a look at the tests. Probably `hip-fpie-option.hip` is unrelated to 
MIPS?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Sylvestre Ledru via Phabricator via cfe-commits]

sylvestre.ledru added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:177
 
+* CMake ``-DCLANG_DEFAULT_PIE_ON_LINUX=ON`` is now the default.
+  This matches GCC installations on many Linux distros.

it would be nice to explain what "pie" it is doing :)



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a subscriber: atanasyan.
MaskRay added a comment.

@atanasyan A few mips test/Driver tests will fail. Wonder if you have time 
making them more portable... Otherwise I'll just add `UNSUPPORTED: 
default-pie-on-linux` to them.

  Failed Tests (6):
Clang :: Driver/hip-fpie-option.hip
Clang :: Driver/mips-cs.cpp
Clang :: Driver/mips-fsf.cpp
Clang :: Driver/mips-img-v2.cpp
Clang :: Driver/mips-img.cpp
Clang :: Driver/mips-mti-linux.c


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120305/new/

https://reviews.llvm.org/D120305

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120305: [Driver] Default CLANG_DEFAULT_PIE_ON_LINUX to ON

[Fangrui Song via Phabricator via cfe-commits]

MaskRay created this revision.
MaskRay added reviewers: foutrelis, sylvestre.ledru, thesamesam, tstellar.
Herald added a subscriber: mgorny.
MaskRay requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

Default the option introduced in D113372  to 
ON to match all(?) major Linux
distros. This improves behavior consistency with GCC.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D120305

Files:
  clang/CMakeLists.txt
  clang/docs/ReleaseNotes.rst
  clang/test/Driver/hexagon-toolchain-elf.c
  llvm/utils/gn/secondary/clang/include/clang/Config/BUILD.gn
  utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h


Index: utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h
===
--- utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h
+++ utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h
@@ -23,7 +23,7 @@
 #define BUG_REPORT_URL "https://github.com/llvm/llvm-project/issues/;
 
 /* Default to -fPIE and -pie on Linux. */
-#define CLANG_DEFAULT_PIE_ON_LINUX 0
+#define CLANG_DEFAULT_PIE_ON_LINUX 1
 
 /* Default linker to use. */
 #define CLANG_DEFAULT_LINKER ""
Index: llvm/utils/gn/secondary/clang/include/clang/Config/BUILD.gn
===
--- llvm/utils/gn/secondary/clang/include/clang/Config/BUILD.gn
+++ llvm/utils/gn/secondary/clang/include/clang/Config/BUILD.gn
@@ -9,7 +9,7 @@
   output = "$target_gen_dir/config.h"
   values = [
 "BUG_REPORT_URL=https://github.com/llvm/llvm-project/issues/;,
-"CLANG_DEFAULT_PIE_ON_LINUX=",
+"CLANG_DEFAULT_PIE_ON_LINUX=1",
 "CLANG_DEFAULT_LINKER=",
 "CLANG_DEFAULT_STD_C=",
 "CLANG_DEFAULT_STD_CXX=",
Index: clang/test/Driver/hexagon-toolchain-elf.c
===
--- clang/test/Driver/hexagon-toolchain-elf.c
+++ clang/test/Driver/hexagon-toolchain-elf.c
@@ -1,3 +1,4 @@
+// UNSUPPORTED: default-pie-on-linux
 // 
-
 // Test standard include paths
 // 
-
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -174,6 +174,9 @@
 Build System Changes
 
 
+* CMake ``-DCLANG_DEFAULT_PIE_ON_LINUX=ON`` is now the default.
+  This matches GCC installations on many Linux distros.
+
 AST Matchers
 
 
Index: clang/CMakeLists.txt
===
--- clang/CMakeLists.txt
+++ clang/CMakeLists.txt
@@ -245,7 +245,7 @@
 set(CLANG_SPAWN_CC1 OFF CACHE BOOL
 "Whether clang should use a new process for the CC1 invocation")
 
-option(CLANG_DEFAULT_PIE_ON_LINUX "Default to -fPIE and -pie on Linux" OFF)
+option(CLANG_DEFAULT_PIE_ON_LINUX "Default to -fPIE and -pie on Linux" ON)
 
 # TODO: verify the values against LangStandards.def?
 set(CLANG_DEFAULT_STD_C "" CACHE STRING


Index: utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h
===
--- utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h
+++ utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h
@@ -23,7 +23,7 @@
 #define BUG_REPORT_URL "https://github.com/llvm/llvm-project/issues/;
 
 /* Default to -fPIE and -pie on Linux. */
-#define CLANG_DEFAULT_PIE_ON_LINUX 0
+#define CLANG_DEFAULT_PIE_ON_LINUX 1
 
 /* Default linker to use. */
 #define CLANG_DEFAULT_LINKER ""
Index: llvm/utils/gn/secondary/clang/include/clang/Config/BUILD.gn
===
--- llvm/utils/gn/secondary/clang/include/clang/Config/BUILD.gn
+++ llvm/utils/gn/secondary/clang/include/clang/Config/BUILD.gn
@@ -9,7 +9,7 @@
   output = "$target_gen_dir/config.h"
   values = [
 "BUG_REPORT_URL=https://github.com/llvm/llvm-project/issues/;,
-"CLANG_DEFAULT_PIE_ON_LINUX=",
+"CLANG_DEFAULT_PIE_ON_LINUX=1",
 "CLANG_DEFAULT_LINKER=",
 "CLANG_DEFAULT_STD_C=",
 "CLANG_DEFAULT_STD_CXX=",
Index: clang/test/Driver/hexagon-toolchain-elf.c
===
--- clang/test/Driver/hexagon-toolchain-elf.c
+++ clang/test/Driver/hexagon-toolchain-elf.c
@@ -1,3 +1,4 @@
+// UNSUPPORTED: default-pie-on-linux
 // -
 // Test standard include paths
 // -
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++