[PATCH] D124493: Move Sanitizer metadata to be on-GlobalValue.

[Mitch Phillips via Phabricator via cfe-commits]

hctim abandoned this revision.
hctim added a comment.
Herald added a subscriber: Enna1.

Integrated slowly and surely as part of the stack leading up to D127911 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124493

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

[PATCH] D124493: Move Sanitizer metadata to be on-GlobalValue.

[Mitch Phillips via Phabricator via cfe-commits]

hctim planned changes to this revision.
hctim added a comment.

Pulled out the IR-specific changes to D126100 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124493

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

[PATCH] D124493: Move Sanitizer metadata to be on-GlobalValue.

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

I've always viewed the current implementation as a hack, and believed this data 
should live in debug info.

It can be convenient to get symbolized reports for global overflow bug in a 
stripped binary, but those bugs are quite rare, and "symbolization" only 
affects the global itself, not the stack trace of the bad memory access. It 
does not seem worth it to support two code paths just for this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124493

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

[PATCH] D124493: Move Sanitizer metadata to be on-GlobalValue.

[Kirill Stoimenov via Phabricator via cfe-commits]

kstoimenov added inline comments.



Comment at: clang/lib/CodeGen/SanitizerMetadataFactory.h:33
+
+class SanitizerMetadataFactory {
+  SanitizerMetadataFactory(const SanitizerMetadataFactory &) = delete;

Not sure if this class follows the 'factory' design pattern, which typically it 
would have some sort of 'produce' or 'create' method. Maybe 
SanitizerMatadataWriter or SanitizerMatadataCreator? 



Comment at: clang/lib/CodeGen/SanitizerMetadataFactory.h:46-48
+  void setASanSpecificMetadata(llvm::GlobalVariable::SanitizerMetadata ,
+   llvm::GlobalVariable *GV, SourceLocation Loc,
+   QualType Ty, bool IsDynInit = false);

This could be private I think.



Comment at: compiler-rt/lib/asan/asan_globals.cpp:90
+
+  DataInfo info;
+  Symbolizer::GetOrInit()->SymbolizeData(g.beg, );

Outside of the scope of this change. It looks like DataInfo's constructor is 
calling memset on it's fields. I would have probably designed this differently 
because now it looks like potential uninitialized when we check for 'info.line 
!= 0'. 



Comment at: llvm/include/llvm/AsmParser/LLToken.h:205
   kw_nest,
+  kw_no_sanitize,
+  kw_no_sanitize_address,

Why do we need to add these to lex? I am surprised that didn't have the need so 
far. 



Comment at: llvm/include/llvm/IR/GlobalValue.h:312
+void RemoveSanitizer(GlobalSanitizer S) { Sanitizer &= ~S; }
+bool HasSanitizer(GlobalSanitizer S) const {
+  if (Sanitizer == GlobalSanitizer::NoSanitize)

I suspect that there might be another place where that logic exists. I am 
worried that we are duplicating it. Do you might looking for it in the code 
base?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124493

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

[PATCH] D124493: Move Sanitizer metadata to be on-GlobalValue.

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka added inline comments.



Comment at: clang/lib/CodeGen/SanitizerMetadataFactory.h:6
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//

vitalybuka wrote:
> can you please either revert 
ignore this one



Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:1352
(VE.getValueID(GV.getInitializer()) + 1));
 Vals.push_back(getEncodedLinkage(GV));
 Vals.push_back(getEncodedAlign(GV.getAlign()));

all serialization stuff deserve own patch, then rebase the rest on that



Comment at: llvm/lib/IR/AsmWriter.cpp:3550
+Out << ", sanitize_address";
+  if (MD.HasSanitizer(SanitizerMetadata::HWAddress))
+Out << ", sanitize_hwaddress";

it extends IR language, corresponding documentation needs to be updated


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124493

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

[PATCH] D124493: Move Sanitizer metadata to be on-GlobalValue.

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka added a comment.

In D124493#3477432 , @filcab wrote:

> Hi @hctim, thanks for the patch.
> I have one question, though. Do you really need to remove the information you 
> removed?
> Some people might be testing ASan binaries without access to debug info 
> (because it's been stripped or it's not been loaded and is not available for 
> the sanitizers to get symbols from, etc), and having the full global 
> information would be useful for those reports.
>
> Can this be done by having a flag to make clang not emit the source 
> information + the sanitizer patch to get the line and column?
> That way we could keep the current behaviour of emitting more info, and you'd 
> still get your savings + use of debuginfo.
>
> Thank you,
> Filipe

It's going to cost us supporting two implementations of the same thing? It 
would be nice to pick a single approach if existing behavior is not critical.




Comment at: clang/lib/CodeGen/CMakeLists.txt:83
   PatternInit.cpp
-  SanitizerMetadata.cpp
+  SanitizerMetadataFactory.cpp
   SwiftCallingConv.cpp

why do you need this rename?

depending on your response you should either:
1. revert
2. rename as NFC in a separate patch and rebase the rest



Comment at: clang/lib/CodeGen/SanitizerMetadataFactory.h:6
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//

can you please either revert 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124493

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

[PATCH] D124493: Move Sanitizer metadata to be on-GlobalValue.

[Kirill Stoimenov via Phabricator via cfe-commits]

kstoimenov added a comment.

One meta-observation: can you split this into smaller patches?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124493

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